Hi Sean. Thanks for code reviewing my recent commits!
- 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.
- 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