[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]
Re: [tor-bugs] #22279 [Metrics/metrics-lib]: simplify and avoid repetition in ParserHelper methods
#22279: simplify and avoid repetition in ParserHelper methods
---------------------------------+-----------------------------------
Reporter: iwakeh | Owner: karsten
Type: enhancement | Status: needs_review
Priority: Medium | Milestone: metrics-lib 1.8.0
Component: Metrics/metrics-lib | Version:
Severity: Normal | Resolution:
Keywords: | Actual Points:
Parent ID: | Points:
Reviewer: | Sponsor:
---------------------------------+-----------------------------------
Comment (by iwakeh):
Replying to [comment:14 karsten]:
> Looks like we overlooked the case when a descriptor does not contain a
line, e.g., `dirreq-v3-reqs`. In this case,
`ExtraInfoDescriptorImpl#dirreqV3Reqs` will stay `null`, and
`ParseHelper#convertCommaSeparatedKeyIntegerValueList()` will not get a
validated string from us but `null`. Please find
[https://gitweb.torproject.org/user/karsten/metrics-
lib.git/commit/?h=task-22279-3&id=823fa496ce2b4f32f791fe7fd1859e67af068d3a
this commit in my task-22279-3 branch] with a test and a fix.
Good, that you found that! Should there be a test added for this in
particular?
All looks fine, passes tests and checks. I added a simple test class
directly for DescriptorImpl [https://gitweb.torproject.org/user/iwakeh
/metrics-
lib.git/commit/?h=task-21932-3&id=991dc7bddfe6a6e692a3c580fe318ed01c23844d
here].
Some questions (partially not related to current changes):
1. Rename `getScanner` into `newScanner`, because a new Scanner is created
with every call and this is not a 'getter'?
1. `ExitList.EOL = "\n" = DescriptorImpl.NL` why not use one for all
delimiters in `newScanner` calls? Shouldn't `NL` be defined in
`Descriptor` and be used everywhere, i.e., deprecate `ExitList.EOL` and
replace with release 2.0.0?
1. Make NL the delimiter default that is already set for the Scanner
returned by `newScanner`?
1. The Scanner usage in TorperfResult is essentially getLine() in the two
places. Do Torperf descriptors contain "\r"?
1. DescriptorImpl.setDigestXXX allow empty or null argument. Should there
be a check?
--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/22279#comment:16>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs