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

[Tor Bug Tracker & Wiki]

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

  * status:  needs_revision => needs_review


Comment:

 Replying to [comment:3 asn]:
 > Quick review:

 Thanks again for the 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.

 Yeah, I completely agree.

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

 Heh, yeah, I should have been more careful about that. Tests started
 failing when I started
 testing the PT code. This was a sloppy solution. It's better now.

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

 I thought those comments were more than sufficiently descriptive :)

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

 Nope, you're completely justified in this. I guess we're following stem's
 path.

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

 It reduced line wraps, but that's silly, so now that function only uses
 trans
 >
 > - 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.

 Both? It's actually a TODO. I was more concerned with verifying the
 minimum than I was/am about the max. I'll implement it, but it's not
 essential for this patch. We can entirely pull it out if I don't finish
 it.

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

 I agree. It seemed like a good idea at first...gone.

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

 Result of not removing debugging stuff.

 > Hopefully Aaron can take a look at your branch too.
 >
 > Thanks for all the code!

 Thanks! New branch bug8614_3, rebased and such.


 Also, re: whitespace in Tests, I fixed those too.

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