[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]
Re: [tor-bugs] #9485 [Pluggable transport]: Cleanup of pyptlib internals
#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
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs