[metrics-bugs] #21932 [Metrics/metrics-lib]: Stop relying on the platform's default charset

[Tor Bug Tracker & Wiki]

#21932: Stop relying on the platform's default charset
---------------------------------+-----------------------------------
 Reporter:  karsten              |          Owner:  metrics-team
     Type:  defect               |         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 karsten):

 Replying to [comment:14 iwakeh]:
 > Replying to [comment:13 karsten]:
 > > I just pushed one more commit to that branch to fix a bug.
 >
 > Good, that you found that!

 Indeed.  I guess I would have found it when running Onionoo and metrics-
 web with this metrics-lib version, which I was planning to do today.  But
 always good to find bugs early.  Let's see what else I'll find today!

 > Should there be a test added for this in particular?

 Sure, added.

 > 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].

 Looks good!

 > 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'?

 Good idea, changed.

 > 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?

 This one is tricky.  We shouldn't impose a newline character for all
 descriptor types.  For example, Torperf measurement results use either
 `\n` or `\r\n` as newline character(s), depending on whether they're
 written by Torperf or OnionPerf.  And we don't know what future descriptor
 types will (want to) use.

 But do we need to expose this in the interface at all?  I could imagine
 deprecating `ExitList.EOL` and just using `DescriptorImpl.NL`, but without
 moving `DescriptorImpl.NL` to `Descriptor.NL`.  Should we do that?  We
 could easily do this in 1.9.0.

 > 1. Make NL the delimiter default that is already set for the Scanner
 returned by `newScanner`?

 See above.  We'd have to provide an overloaded method for non-default
 delimiters, and the question is whether that saves so much code or
 complexity overall.  We could do it, but we could just leave it as is.

 > 1. The Scanner usage in TorperfResult is essentially getLine() in the
 two places.  Do Torperf descriptors contain "\r"?

 See above.  But to be honest, I focused mainly on Tor descriptors and just
 tweaked the other descriptors until everything compiled again. ;)  We
 could probably make `TorperfResultImpl` and `ExitListImpl` better if we
 wanted.  But maybe we should save that for 1.9.0?

 > 1. DescriptorImpl.setDigestXXX allow empty or null argument.  Should
 there be a check?

 Do you think that's necessary?  These methods are only called by
 ourselves, and we're already making sure that we're neither passing an
 empty or null argument when calling that method.  I don't feel strongly
 though.

 Okay, I pushed two new commits to
 [https://gitweb.torproject.org/user/karsten/metrics-
 lib.git/log/?h=task-21932-3 my task-21932-3 branch] with new tests and
 renamed `newScanner()` method.  I'll hunt for new bugs now.  If I don't
 find any, should I squash and merge to master?  Or do you want to add more
 changes from your suggestions above?

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