[tor-bugs] #9485 [Pluggable transport]: Cleanup of pyptlib internals

[Tor Bug Tracker & Wiki]

#9485: Cleanup of pyptlib internals
-------------------------------------+---------------------
     Reporter:  infinity0            |      Owner:  asn
         Type:  defect               |     Status:  new
     Priority:  normal               |  Milestone:
    Component:  Pluggable transport  |    Version:
   Resolution:                       |   Keywords:  pyptlib
Actual Points:                       |  Parent ID:
       Points:                       |
-------------------------------------+---------------------

Comment (by infinity0):

 Replying to [comment:3 asn]:
 > Comments after first code review:
 >
 > * Why is `fromEnv()` a classmethod that returns an instance of the
 class? Why don't you do that stuff in the `__init__` of the class? I'd
 prefer if it was so, if there is no real reason for `fromEnv` to be a
 classmethod.
 >
 The abstract reason is to separate parsing vs initialisation which is good
 software engineering practise. This is a common idiom, I am not doing
 anything weird here.

 A concrete reason is to provide an entry point that bypasses all the
 heavyweight env-var parsing. This allows us to create an empty default
 config in a simple way, rather than having to worry about reverse-
 engineering the correct env-var values that, when parsed, turn into an
 empty default config. This shortcut is actually done in test_core.py, as
 well as any potential future code that might want to initialise a Config()
 without going through env-vars.

 If you don't consider this to be a "real reason" - why not? Having a
 separate __init__ is only about 20 more lines total; if I remove this then
 I would have to add even more lines to the tests, including duplicating a
 lot of the code in test_core.py into server- and client-specific versions.
 I'll make this change if you insist, but I maintain that it adds
 complexity even if it looks shorter.

 > * `env_id` is unused.
 >
 Done, removed.

 > * Why do you call the function `getServedTransports`? Why ''Served''?
 Same with `getServedBindAddresses`. Served by whom and where? I think I
 would prefer `getTransports` or something.
 >
 This is (to quote the new docstring) "transports that this plugin can
 serve", as opposed to transports requested by Tor in the
 TOR_PT_*_TRANSPORTS environment variable (which is a superset of it). I
 can make the name change if you still prefer it, though.

 > * Can you make it clearer that `getServedBindAddresses` returns a dict?
 Also, how are people supposed to get the address of the ExtendedORPort, or
 the state location with the new API? Can this be mentioned in the sphinx
 docs?
 >
 Done former, already did latter.

 > * You didn't update the docs for the `reportMethodSuccess` API change.
 >
 Done.

 > * In `EnvError`, is the following correct? Doesn't that return a
 boolean?
 > {{{
 >     def __str__(self):
 >         return self.message or self.cause.message
 > }}}
 >
 `a or b` is short for `a if bool(a) else b`

 > * Please do the :raises: doc dance in exception-raising places like
 `CheckClientMode` and `env_has_k`.
 >
 Done.

 > * Have you used pylint (or any other static analyzer) on the new code?
 Did it find anything fix worthy (unused vars, unused imports, etc.)
 >
 Done.

 > Apart from the above, we need to write patches for obfsproxy and
 sshproxy, and then we are ready to merge :)
 I'll prepare those patches, but they should only be merged *after* this is
 already deployed.

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