Code review comment for lp:~frankban/juju-quickstart/env-manage-selection-view

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

LGTM with mostly trivial comments (discussion on possible decoys is the
only thing that ended up being interesting).

https://codereview.appspot.com/43860044/diff/1/quickstart/cli/base.py
File quickstart/cli/base.py (right):

https://codereview.appspot.com/43860044/diff/1/quickstart/cli/base.py#newcode37
quickstart/cli/base.py:37: _Application = namedtuple(
Nice improvement.

https://codereview.appspot.com/43860044/diff/1/quickstart/cli/views.py
File quickstart/cli/views.py (right):

https://codereview.appspot.com/43860044/diff/1/quickstart/cli/views.py#newcode140
quickstart/cli/views.py:140: detail_view = functools.partial(
nice :-)

I toyed with suggesting that you pass env_index to env_detail, and vice
versa, but naah for now.

https://codereview.appspot.com/43860044/diff/1/quickstart/cli/views.py#newcode144
quickstart/cli/views.py:144: envs.get_env_data(env_db, env_name)
Actually getting the data here seems dangerous to me, since each of
these include a "default" flag which is actually determined by the
env_db collectively. As a pattern, I think it would be better to have
env_name only sorted, or possibly env_name mapping to thunks. I haven't
seen whether this actually affects your code yet, but it feels like a
bug magnet.

[Later] Eh, never mind. This is not a long-lived data structure. It's
fine. :-)

https://codereview.appspot.com/43860044/diff/1/quickstart/cli/views.py#newcode180
quickstart/cli/views.py:180: return env_db, None
this is a decoy, right? No one can use these?

I worry a bit that the functional style here is a mismatch with Urwid.
These functions are all about side effects. However, on reflection, I
feel like it's easy to read what you have, and that increasing the
functional feel is increasing the simplicity. We'll see how time and
other people instruct us, I guess. :-)

https://codereview.appspot.com/43860044/diff/1/quickstart/cli/views.py#newcode244
quickstart/cli/views.py:244: return env_db, None
Isn't this a decoy? That is, no code will ever actually pay attention
to these results? either index_view will get the mutated result in
set_default, or the urwid caller will get it via use?

https://codereview.appspot.com/43860044/

« Back to merge proposal