Code review comment for lp:~frankban/lpsetup/search-interface2

Revision history for this message
Gary Poster (gary) wrote :

This looks very nice Francesco.

I only have trivial comments.

(1) Maybe test name changes for the new behavior?
  test_short -> test_output_of_single_interface (or test_output_of_explicit_interface)?
  test_verbose -> test_output_of_all_interfaces?

(2) This docstring of the "stopped" contextmanager has one small error (temporary -> temporarily). I'd prefer a rewrite of the second sentence, but some small punctuation changes would improve it.

+ """A context manager to temporary stop the given `obj`.
+
+ Entering this context manager `obj.stop()` is called, while
+ `obj.start()` is invoked exiting.

So, the first sentence should be "A context manager to temporarily stop the given `obj`."

Here's my suggestion for the second sentence: "When entering this context manager, `obj.stop()` is called.
`obj.start()` is invoked when exiting."

Alternatively, this smaller change would be ok: "Entering this context manager, `obj.stop()` is called; while `obj.start()` is invoked exiting."

As an aside, I can't help but have a hunch that the code's structure would have been different had we not started with -s, but I can't see another much better way of doing things, and this is nice and clear, so I have no concerns or suggestions.

review: Approve

« Back to merge proposal