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

[Damian Johnson]

Hi Sean. Thanks for code reviewing my recent commits!

> 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 assertions were opt-in (ie, only triggered if the caller sets
assert_class or assert_content) so I doubt that they would be a
surprise for test writers. That said, I went ahead and reverted it. I
agree that mixing those assertions with the event constructor was a
little clunky, and it really didn't end up having the readability
improvements I hoped it would.

> 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?

The reason I did this for kwargs was because it was necessary to avoid
having new spec additions break us. If a new quoted positional
argument appeared then the parser would ignore it, but a quoted
keyword arg would break all prior args. For instance...

========================================

class MyEvent(Event):
  _POSITIONAL_ARGS = ("foo")
  _KEYWORD_ARGS = {
    'bar': 'bar',
  }

========================================

MY_EVENT arg1 "quoted arg" bar=stuff

... would be parsed as...

foo = arg1
bar = stuff
positional_args = ['arg1', '"quoted', 'arg"']
keyword_args = {'bar': 'stuff'}

========================================

MY_EVENT arg1 bar=stuff blarg="quoted arg"

... would be parsed as...

foo = arg1
bar = None
positional_args = ['arg1', 'bar=stuff', 'blarg="quoted', 'arg"']
keyword_args = {}

========================================

This is because the parser would see the last bit ('arg"') and
conclude that it was a positional argument since it didn't match the
key=value pattern. This isn't strictly wrong according to the spec (it
makes no allowances for new additions being quoted), though that's
probably just an oversight.

I wouldn't mind for positional arguments to also check for quoted
values. I was simply avoiding that because screwy situations could
then break us. For instance...

MY_EVENT "arg1 arg2"

... where the spec says we really *do* have two non-quoted positional
arguments. That said, if we ever saw such a thing I'd probably
conclude that tor was *trying* to confuse us. :P

Patch welcome.

> 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.

It's not being restricted. The expected_signals is only used so that
we log if we get something else. It doesn't prevent us from having
other values - I'd just like to know if we get them since the pydocs
and spec would then need to be tweaked.

> 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.

Sweet! Mind sending me a link to the coverage tool? I'd like to see
which modules are lacking coverage.

Cheers! -Damian