[tor-dev] Stem code review 2012-12-04

[Sean Robinson]

Stem devs,

I did a code review of recent Stem commits ("Tor event handling" merge
(commit 42872dd08e81d6b3) through "Checking for None by identity" (commit
69f72efc9367092c)).  My comments and questions follow.  I will skip my own
contributions (they were great 8-) in that range, as I am biased.

Event._log_if_unrecognized() is a good idea that vastly improves the
readability of Event subclasses and reduces code duplication.

Regarding commit fb0aec5d95e9d2e6 "tidying up boilerplate":
1) I do not like the new _get_event() with assert_class and
assert_content.  There are transformations and tests and returned values
all within what is a mock object builder, meaning it works via
side-effect.  This could be surprising to test writers.
2) I vote to keep "self.assertTrue(isinstance(event,
stem.response.events.StatusEvent))" style tests after producing the event.

The quoted key/value mapping is more readable, now.  Good work.  Why not
look for quoted positional args before non-quoted positional args?  Why not
do just like kwarg handling?

Why restrict SignalEvents to expected_signals when control-spec.txt allows
more?  This may mean changes later to add support for things the protocol
already claims to support.

I set up coverage.py for another project and I wondered if it would work
with Stem.  So, I ran "coverage run --parallel-mode --branch --omit="test*"
./run_tests.py -u -i -t RUN_NONE" in the stem directory.  The results are
66% - 100% coverage per module.  Another impressive accomplishment.  And
this is only running a subset of the possible tests.


-- 
Sean Robinson
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.torproject.org/pipermail/tor-dev/attachments/20121207/cae4037a/attachment.html>