Code review comment for lp:~hazmat/pyjuju/maas-with-tags

ux
Thanks for the review.

On Wed, Feb 20, 2013 at 12:50 PM, <email address hidden> wrote:

> Fix LGTM. The one thing I'm not clear on is if there's still any route
> for juju to convert to a set, and stash the constraint somewhere, losing
> the original string. As far as I can see not, as there's no
> deserialisation function.
>

constraints are stored in original form passed post convert/validation on
state objects, and resurrected via subsequent constraint set parse, ie the
original string information is never lost or discarded.

>
>
> https://codereview.appspot.**com/7365044/diff/1/juju/**
> providers/maas/maas.py<https://codereview.appspot.com/7365044/diff/1/juju/providers/maas/maas.py>
> File juju/providers/maas/maas.py (right):
>
> https://codereview.appspot.**com/7365044/diff/1/juju/**
> providers/maas/maas.py#**newcode151<https://codereview.appspot.com/7365044/diff/1/juju/providers/maas/maas.py#newcode151>
> juju/providers/maas/maas.py:**151: params[key_to] = translate(value,
> key_from, constraints)
> Should we make the translate functions take (constraint, key_from)
> instead? Then rather than doing the get above, it would just be:
>
> value = translate(constraint, key_from)
>
> if value is not None:
> params[key_to] = value
>
> Not sure if that's a simplification overall, but would really like to
> lose some of the complexity here.

...and thinking about it, makes the translation functions deal with the
> hasattr case. Can instead this check for the key without doing the
> get/conversion step?
>
>
not sure this is really adding anything, feels stylistic. the hasattr case
is handled outside of the value converter with the is None check.

> https://codereview.appspot.**com/7365044/diff/1/juju/**
> providers/maas/tests/test_**launch.py<https://codereview.appspot.com/7365044/diff/1/juju/providers/maas/tests/test_launch.py>
> File juju/providers/maas/tests/**test_launch.py (right):
>
> https://codereview.appspot.**com/7365044/diff/1/juju/**
> providers/maas/tests/test_**launch.py#newcode124<https://codereview.appspot.com/7365044/diff/1/juju/providers/maas/tests/test_launch.py#newcode124>
> juju/providers/maas/tests/**test_launch.py:124: class
> FakeWithTags(**FakeMAASHTTPConnection):
> I'd probably write this test with a reusable Fake class, and avoid
> having closure/assertions in it, but this certainly improves our
> coverage so is a good step forwards.
>

given a lack of reuse atm, again feels stylistic.

>
> https://codereview.appspot.**com/7365044/diff/1/juju/**
> providers/maas/tests/test_**maas.py<https://codereview.appspot.com/7365044/diff/1/juju/providers/maas/tests/test_maas.py>
> File juju/providers/maas/tests/**test_maas.py (right):
>
> https://codereview.appspot.**com/7365044/diff/1/juju/**
> providers/maas/tests/test_**maas.py#newcode401<https://codereview.appspot.com/7365044/diff/1/juju/providers/maas/tests/test_maas.py#newcode401>
> juju/providers/maas/tests/**test_maas.py:401: constraints =
> constraints.with_series('**splendid')
> I really liked these tests being actual unit tests that didn't need the
> whole provider setup... but I think we're cursed with juju to have
> everything depend on everything else.
>

yes, tests that actually verify do need additional context, else we end up
the back in the poorly tested scenario that caused the numerous issues and
revisions that attempted to address the underlying tags issues.