Re: [tor-bugs] #8614 [BridgeDB]: BridgeDB should be able to return multiple transport types at the same time

["Tor Bug Tracker & Wiki" <blackhole@xxxxxxxxxxxxxx>]

#8614: BridgeDB should be able to return multiple transport types at the same time
----------------------+-----------------------------------------------------
 Reporter:  asn       |          Owner:                
     Type:  defect    |         Status:  needs_revision
 Priority:  normal    |      Milestone:                
Component:  BridgeDB  |        Version:                
 Keywords:            |         Parent:  #8615         
   Points:            |   Actualpoints:                
----------------------+-----------------------------------------------------
Changes (by asn):

  * status:  new => needs_revision


Comment:

 Quick review:

 - The code of BridgeDB is largely undocumented; when you change an
 undocumented function, can you add a tiny amount of documentation on what
 it does? In the long run, this process is going to improve the code
 quality considerably.

 - Why did you teach `parseORAddressLine` about transports? Isn't it
 supposed to just parse OR addresses? It seems that you are feeding it the
 output of `getConfigLine()`, that's why. Either the function name is
 wrong, or that function does more than it's supposed to do.

 - Try to do more helpful documentation. Examples:
 {{{
 # Order the list of bridges such that some prefix satisfies a requirement
 # for certain pluggable transports
 }}}
 What is the "prefix" in this case?

 {{{
 # Check that we are returning the specified number of transports
 # in this list of bridges
 }}}
 what is the "specified number"? what is the 'remain' argument?

 {{{
         # Let's grab a few extra, just in case
 }}}
 "just in case" what?

 Sorry for bothering you about comments, but the BridgeDB code is already
 undocumented and confusing, and we shouldn't convolute it further (if we
 want it to remain maintainable in the future). If you are looking for tips
 on Python function documentation, check out other projects (like 'stem' if
 you want a paranoid approach). IMO, documentation is extremely important
 in dynamically-typed languages, otherwise the code becomes unmaintainable
 very easily.

 - In `checkTransportRequirement()` you do  `trans = transports` and work
 with `trans` from that point and on. That's an unusual pattern.

 - In the `needTransports` tuple, while you can specify a nonexistent
 `maxcount`, you can't do the same for `mincount`. Is this a feature or a
 bug? Also, DOCDOC.

 - f032596fc9e320d87b18f3cd88a7559cd8eccf7f might be an overkill. The user
 never requests a specific number of bridges; bridgedb returns a number of
 bridges in a best-effort basis.
 Even if you wanted to notify the user that we don't have enough bridges in
 the database, the exception-based approach you are taking might be a bit
 too messy. While your approach is not wrong, if in the future you wanted
 to notify the user, I think you could have achieved the same result with a
 `warnTheUserAboutBridgesScarcity()` callback from within
 `getNumBridgesPerAnswer()`. In any case, if you like your approach, stick
 with it.

 - In the beginning of `testMeetRequirementWithMixedSet2()` you do some
 things with the `types` dictionary and then you initialize it back to the
 empty dictionary again.

 Hopefully Aaron can take a look at your branch too.

 Thanks for all the code!

-- 
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/8614#comment:3>
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