Stem devs,
This is a review of Stem commits from 2012-12-11 through 2012-12-21.
Damian, you are very good at documentation, concise and informative. I tend to hit one or the other, but not both. I recognize when someone gets the right mix. Good work.
I, too, have found logging.basicConfig[0] hides too much about how it works and, now, I roll my own logging handler early.
Would GETINFO/GETCONF cache hit logging[1] benefit from log_once? Or even something between log and log_once (e.g. rate_limited_log)?
Is there any particular reason for moving away from readthedocs.org? I'm curious if there was more than a desire to self-host as much as possible.
Good work on the Controller.get_conf() clean-up[2]. This is easier to code against with the predictable return types. I have already written a new method with this and always getting a list (event empty) reduces the amount of error checking I had to do.
The heartbeat time tracking is a good idea[3]. But, I wonder, should this bother with an accessor method? Could this work as just well as an (non-callable) attribute?
[0]: https://gitweb.torproject.org/stem.git/commit/a7b275a3d64301da4d23137dbadd48... [1]: https://gitweb.torproject.org/stem.git/commit/9b5ff19be35ebbf8c75772a63406ba... [2]: https://gitweb.torproject.org/stem.git/commit/5b45d3125c811067d7c3ba0a11e324... [3]: https://gitweb.torproject.org/stem.git/commit/6bc92816dc4356a204fdde0f160ee3...
Hi Sean, thanks for the code review!
Would GETINFO/GETCONF cache hit logging[1] benefit from log_once? Or even something between log and log_once (e.g. rate_limited_log)?
Interesting thought, but I don't think so. When I look for cache hits it's usually to answer the question of 'did doing activity X result in requesting parameter Y'. Having stem log once or drop logging when it exceeds a certain rate would make that more confusing.
Is there any particular reason for moving away from readthedocs.org? I'm curious if there was more than a desire to self-host as much as possible.
I talked a bit about the reason for moving on 'https://trac.torproject.org/7324'. Mostly the reasons were (in order by how much I cared about them)...
1. A shorter and more official sounding tpo domain.
2. The daily autobuilds on read-the-docs were always stuck, so the documentation was only updated when I manually built it. This meant that we usually had stale docs. Now that we're on our own hardware I also bumped the rate of checking for new content so the site updates within five minutes of a push.
3. Self-hosting provides better security and visitor privacy. I didn't experiment to see how good the sandboxing of read-the-docs builds were, but a community system that runs arbitrary code is a little spooky.
The heartbeat time tracking is a good idea[3]. But, I wonder, should this bother with an accessor method? Could this work as just well as an (non-callable) attribute?
I'd be fine with that. The only advantage I can think for having it as a method is if we want custom handling in certain situations (for instance, returning None if we're disconnected). However, we don't presently do that so I could go either way.