Code review comment for lp:~bcsaller/pyjuju/sane_output_test_option

Revision history for this message
Kapil Thangavelu (hazmat) wrote :

I'm still a little unclear as to the desired behavior outcome and its
testing with this branch, as it currently tested it doesn't seem
particularly useful.

https://codereview.appspot.com/6216053/diff/6001/juju/hooks/commands.py
File juju/hooks/commands.py (right):

https://codereview.appspot.com/6216053/diff/6001/juju/hooks/commands.py#newcode18
juju/hooks/commands.py:18: class TestOptionMixin(object):
missing doc string describing purpose/usage.

https://codereview.appspot.com/6216053/diff/6001/juju/hooks/commands.py#newcode34
juju/hooks/commands.py:34: self.handle_test(result)
there's a lot of unesc. method delegation here. just inline the
test_true/handle_test behavior.

The use of the mixin, also feels like its adding complexity here, around
subclass ordering and proper lineage to customize_parser vs. render
(which have inverse call lineages). Incorporating into the base class
might be cleaner via class flag switch.

https://codereview.appspot.com/6216053/diff/6001/juju/hooks/commands.py#newcode114
juju/hooks/commands.py:114: def render(self, result):
Why is this returning None, why is it implemented at all? ditto for the
two other implementations of the same here.

https://codereview.appspot.com/6216053/diff/6001/juju/hooks/tests/test_invoker.py
File juju/hooks/tests/test_invoker.py (right):

https://codereview.appspot.com/6216053/diff/6001/juju/hooks/tests/test_invoker.py#newcode356
juju/hooks/tests/test_invoker.py:356:
this is testing something very different then what the cli usage would
commonly be, namely the cli usage would be testing for values unset.

For values set, a null value seems valid, has the go team documented the
behavior of this cli param?

https://codereview.appspot.com/6216053/

« Back to merge proposal