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.
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
On Fri, Dec 7, 2012 at 8:01 PM, Damian Johnson atagar@torproject.orgwrote:
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?
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.
I must not be reasoning about Event._parse_standard_attr() correctly. I think it is already looking for _QUOTED positional args, but is working at it backwards from _KEYWORD_ARGS parsing.
Well, I already wanted to write unit tests for Event. So, I'll try exercising the class and see what I can see.
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.
While preparing to argue further, I see the mistake I made: SIGNAL command versus SIGNAL event. I was reading the wrong part of the spec. Please disregard.
I set up coverage.py for another project and I wondered if it would work
with Stem.
Sweet! Mind sending me a link to the coverage tool? I'd like to see which modules are lacking coverage.
http://nedbatchelder.com/code/coverage/ http://pypi.python.org/pypi/coverage
I've used this on three projects (not counting Stem) and it helps check that written tests are reaching all the dark crevices of one's code. If you have questions, I'm happy to help. Once installed, do:
1) coverage run --parallel-mode --branch --omit="test*" ./run_tests.py -u -i -t RUN_NONE (or some variation) 2) coverage combine 3) coverage html 4) read the nice reports in the htmlcov/ directory
I must not be reasoning about Event._parse_standard_attr() correctly. I think it is already looking for _QUOTED positional args, but is working at it backwards from _KEYWORD_ARGS parsing.
Right. The thing that I'm talking about is *new* quoted arguments (ie, things not presently in the spec and hence not in _POSITIONAL_ARGS, _KEYWORD_ARGS, or _QUOTED). Presently positional and keyword work a little differently from each other for those new additions...
* With positional arguments we only parse them as being quoted if they're in _QUOTED. Hence new positional additions that are quoted will result multiple values in positional_args until we update the parser.
* With keyword arguments _QUOTED is not consulted. Instead we determine if they're quoted or not based on which regex they match.
The former is a strict parsing approach, where we're assured to parse the things we presently support correctly but new additions could put mis-parsed content in our positional_args.
The later is a more permissive parsing approach where it's possible (though highly unlikely) that some content could confuse us but we more gracefully handle new additions.
Probably the best of both worlds would be to use the strict approach for things we presently parse, and a permissive approach for new additions. Ie...
if len(unparsed_positional_args) > len(_POSITIONAL_ARGS): # Get the positional argument content that isn't recognized by # our parser. This probably represents a new addition to the # spec, and might or might not be quoted values.
unrecognized_content = " ".join(unparsed_positional_args[len(_POSITIONAL_ARGS):])
# TODO: Try to parse out quoted values, falling back to # parse them as being unquoted.
... then move on to the present parsing for _POSITIONAL_ARGS
Hope that helps. -Damian
-
Damian Johnson -
Sean Robinson