#6946: Allow filtering relays by OS
-----------------------------+--------------------------------
 Reporter:  rransom          |          Owner:  irl
     Type:  enhancement      |         Status:  needs_revision
 Priority:  Medium           |      Milestone:
Component:  Metrics/Onionoo  |        Version:
 Severity:  Normal           |     Resolution:
 Keywords:                   |  Actual Points:
Parent ID:                   |         Points:
 Reviewer:  karsten          |        Sponsor:
-----------------------------+--------------------------------
Changes (by karsten):

 * status:  needs_review => needs_revision


Comment:

 Some things I noticed:
  - You're reusing `ResourceServlet#parseVersionParameter` for parsing the
 `os` parameter. Wouldn't it make more sense to reuse
 `#parseContactParameter` here? After all, we'll want to support searches
 for `"Linux"` as well as for `"Windows 95"` here, right? Also, maybe
 rename the method to `#parseContactOrOsParameter` to make it clear that
 the method is used for both.
  - Did you try out what happens when you search for strings that contain
 spaces? And did you also try out using the new `"os"` parameter in a
 qualified search term? IIRC, we did not fix the space issue in qualified
 search terms, and in this case it may bite us again. I don't have an easy
 fix for this.
  - Do you mind extending `ResourceServletTest` to actually test different
 requests using the new parameter? Maybe the `#testContactXY` methods serve
 as a template.

 I did not run this code yet. I can do that later today, unless there's
 revised code that I can test instead.

 Thanks!

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/6946#comment:15>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online
_______________________________________________
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Reply via email to