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

Revision history for this message
Richard Harding (rharding) wrote :

Code looks good with a couple of readable enhancing notes. The UI code
is a bit dense as someone that's not looked at it before so will trust
in QA.

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(
why the _? Why call it 'App' but also call it _Application? In my
experience I'd just have App = namedtuple('App'...

https://codereview.appspot.com/43860044/diff/1/quickstart/cli/base.py#newcode152
quickstart/cli/base.py:152: # place where to add new API functions,
after changing the application
Create the App named tuple. If, in the future, we have a view that
requires additional capabilities or API access, this is the place to add
those.

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

https://codereview.appspot.com/43860044/diff/1/quickstart/cli/ui.py#newcode49
quickstart/cli/ui.py:49: None: 'selected',
I don't follow this. When None is in focus it's selected?

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#newcode32
quickstart/cli/views.py:32: A view is a callable receiving an app object
(a named tuple of functions) and
Can we capitalize the App here and in cases referencing the specific
object?

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

« Back to merge proposal