[tor-bugs] #12882 [Onionoo]: Logging Framework

[Tor Bug Tracker & Wiki]

#12882: Logging Framework
-----------------------------+--------------------------
     Reporter:  iwakeh       |      Owner:  iwakeh
         Type:  enhancement  |     Status:  needs_review
     Priority:  normal       |  Milestone:
    Component:  Onionoo      |    Version:
   Resolution:               |   Keywords:
Actual Points:               |  Parent ID:
       Points:               |
-----------------------------+--------------------------

Comment (by karsten):

 Hmm, I looked through the first few hundred lines of 0001-whitespace-
 changes.patch​, and those changes don't confirm to what I intended to
 write above.  Here are some examples:

 {{{
 diff --git a/src/main/java/org/torproject/onionoo/cron/Main.java
 b/src/main/java/org/torproject/onionoo/cron/Main.java
 index bd4b95a..3fb355f 100644
 --- a/src/main/java/org/torproject/onionoo/cron/Main.java
 +++ b/src/main/java/org/torproject/onionoo/cron/Main.java
 @@ -26,7 +26,7 @@ public class Main {
        Logger.printStatusTime("Acquired lock");
      } else {
        Logger.printErrorTime("Could not acquire lock.  Is Onionoo "
 -          + "already running?  Terminating");
 +        + "already running?  Terminating");
        return;
      }

 @@ -68,10 +68,9 @@ public class Main {
        Logger.printStatusTime("Released lock");
      } else {
        Logger.printErrorTime("Could not release lock.  The next "
 -          + "execution may not start as expected");
 +        + "execution may not start as expected");
      }

      Logger.printStatus("Terminating.");
    }
  }
 -
 }}}

 The first two changes should go away, because those lines are
 continuations of the previous line, and therefore should use 4 spaces, not
 2.  The last change should go away, because files should end with two
 newlines.

 {{{
 diff --git
 a/src/main/java/org/torproject/onionoo/docs/BandwidthDocument.java
 b/src/main/java/org/torproject/onionoo/docs/BandwidthDocument.java
 index ea20a5e..021c2f7 100644
 --- a/src/main/java/org/torproject/onionoo/docs/BandwidthDocument.java
 +++ b/src/main/java/org/torproject/onionoo/docs/BandwidthDocument.java
 @@ -8,20 +8,22 @@ public class BandwidthDocument extends Document {

    @SuppressWarnings("unused")
    private String fingerprint;
 +
    public void setFingerprint(String fingerprint) {
      this.fingerprint = fingerprint;
    }

 }}}

 This additional newline shouldn't be there, because the setter is
 "grouped" together with its attribute by omitting the newline.  Does that
 make sense, or do you prefer if we change that?

 {{{
    public void clearDirty() {
      this.isDirty = false;
    }

 -  private SortedMap<Long, long[]> writeHistory =
 -      new TreeMap<Long, long[]>();
 +  private SortedMap<Long, long[]> writeHistory
 +    = new TreeMap<Long, long[]>();
 +
    public void setWriteHistory(SortedMap<Long, long[]> writeHistory) {
      this.writeHistory = writeHistory;
    }
 }}}

 Apart from the 2 spaces which should be 4, I don't have a clear preference
 whether the `=` should be on the first or second line.  We can change that
 if you want.

 {{{
 -          ? endMillis : history.tailMap(startMillis).firstKey();
 -      if (previousEndMillis <= startMillis &&
 -          nextStartMillis >= endMillis) {
 -        history.put(startMillis, new long[] { startMillis, endMillis,
 -            bandwidth });
 +        ? endMillis : history.tailMap(startMillis).firstKey();
 +      if (previousEndMillis <= startMillis
 +        && nextStartMillis >= endMillis) {
 +        history.put(startMillis, new long[]{startMillis, endMillis,
 +          bandwidth});
 }}}

 Similarly, I don't feel strongly about the `&&` being at the line end or
 beginning.  Happy to change that if you prefer.

 What I don't like as much is the `long[]{startMillis,` part which I find
 kinda hard to read.

 Anyway, how about we separate the whitespace discussion from the logging
 part?  I can't apply your patches 0002 to 0004 without 0001.  Would you be
 able to rebase those patches to current master without your 0001 patch,
 and then we move the reformatting discussion to a separate ticket?

 Sorry this is so difficult.  Thanks for your contribution!

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/12882#comment:9>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online