[metrics-bugs] #20765 [Metrics/metrics-lib]: adapt to new lines in votes and consensus and make the adaption to protocol changes easier
Tor Bug Tracker & Wiki
blackhole at torproject.org
Tue Feb 14 08:41:39 UTC 2017
#20765: adapt to new lines in votes and consensus and make the adaption to protocol
changes easier
---------------------------------+-----------------------------------
Reporter: iwakeh | Owner: metrics-team
Type: enhancement | Status: needs_review
Priority: Medium | Milestone: metrics-lib 1.6.0
Component: Metrics/metrics-lib | Version:
Severity: Normal | Resolution:
Keywords: | Actual Points:
Parent ID: | Points:
Reviewer: | Sponsor:
---------------------------------+-----------------------------------
Comment (by karsten):
Replying to [comment:7 iwakeh]:
> Replying to [comment:5 karsten]:
> > And there's now [https://gitweb.torproject.org/user/karsten/metrics-
lib.git/commit/?h=task-20765-2&id=3a990a48ca23ff4c5069d50b170761fadeb49018
another commit] for the second half of the changes above. Please review!
>
> Tests and checks pass.
Thanks for checking!
> For a boolean return value I'd expect "is*" and not "get*", i.e.,
`isSharedRandParticipate` instead of`getSharedRandParticipate`.
Changed.
> ===
>
> One more comment on the previous commit:
>
> The new `private void parse*` methods just set a global variable each;
why not have the `this.* = ParseHelper.parseProtocolVersions(line, line,
parts);` lines in the switch stmt w/o the method calls?
I'm not sure. I think the switch statement is more readable if we just
make one simple method call per line and move the parsing logic to
separate methods. And what if this parsing logic ever gets more
complicated and requires another line; would we have to move that logic to
a new method then? Let's just stick to the current scheme if you don't
mind.
--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/20765#comment:9>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online
More information about the metrics-bugs
mailing list