Stem devs,
This is a review of recent commits to Stem. It begins where my last review ended[0] and finishes at the "Adding a close_stream..." merge.
The pydoc changes to Controller.extend_circuit are good additions.
I do not understand much of the context for changes regarding network status docs, exit policy, address type, etc. But, the code appears to do what the comments claim it should do (e.g. lazy loading address info). Take that evaluation with a grain of salt.
As to re-attaching event listeners[1], I agree that putting a specialized hook into BaseController.msg seems bad. I have an alternate idea[2] that puts the re-attachment in an authenticate method. I am not proposing this as the solution, but I hope this sparks the idea for another way to handle re-attachment.
I think the 'as' import keyword[3] is mainly for shortening long names and easily avoiding namespace collision. For example:
import pkg.reallylongmodulename as rlmn
and
import socket import niftysocket.socket as nsocket
[0]: https://lists.torproject.org/pipermail/tor-dev/2012-December/004240.html [1]: https://gitweb.torproject.org/stem.git/commit/885a294646703a537c37cd2a5ac9aa... [2]: https://gitorious.org/stem-robinson/stem-robinson/commits/exp-reattach-liste... [3]: https://gitweb.torproject.org/stem.git/commit/3da47d3b9d6d1ae5c6b2013a4247c4... [4]: this footnote intentionally left blank
Hi Sean, thanks for the code review!
As to re-attaching event listeners[1], I agree that putting a specialized hook into BaseController.msg seems bad. I have an alternate idea[2] that puts the re-attachment in an authenticate method. I am not proposing this as the solution, but I hope this sparks the idea for another way to handle re-attachment.
I'd like to find a better option, but the trouble is that there's multiple methods for authenticating to a Controller. In particular there's three that come to mind...
1. Using the Controller's authenticate() method. 2. Using stem.connection's authenticate() function. 3. Using the BaseController's msg() function to send an 'AUTHENTICATE' request.
Your suggestion of moving the hook to Controller.authenticate() just covers the first. Mine covers all three but feels a bit icky. Suggestions welcome! -Damian