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

Please take a look.

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

https://codereview.appspot.com/44750044/diff/1/quickstart/cli/forms.py#newcode82
quickstart/cli/forms.py:82: if generate_method is not None:
On 2014/01/02 15:19:28, gary.poster wrote:
> For readability, I wonder if we ought to stick most of this block in a
separate
> function.

> if generate_method is not None:
> widgets.append(
> create_generate_widget(generate_method, edit_widget))

> ...or similar?

Done.

https://codereview.appspot.com/44750044/diff/1/quickstart/cli/forms.py#newcode172
quickstart/cli/forms.py:172: urwid.Text(('error', 'please correct the
errors below')),
On 2014/01/02 15:19:28, gary.poster wrote:
> Nice to have: a count. ("Please correct the 2 errors below")

Done.

https://codereview.appspot.com/44750044/diff/1/quickstart/cli/forms.py#newcode179
quickstart/cli/forms.py:179: # Boolean values as represented as
checkboxes.
On 2014/01/02 15:19:28, gary.poster wrote:
> s/as/are/

> Boolean values are represented a checkboxes

Done.

https://codereview.appspot.com/44750044/diff/1/quickstart/cli/forms.py#newcode208
quickstart/cli/forms.py:208: return ui.create_controls(*controls)
On 2014/01/02 15:05:32, benji1 wrote:
> This is so small it is almost not worth mentioning, but I thought you
might like
> it:

> return ui.create_controls(
> *(ui.MenuButton(caption, callback) for caption, callback in
actions))

> Or if we want to go full-on functional:

> return ui.create_controls(*map(ui.MenuButton, actions))

Done.

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

https://codereview.appspot.com/44750044/diff/1/quickstart/cli/ui.py#newcode49
quickstart/cli/ui.py:49: ('optional status', 'light magenta', 'light
gray'),
On 2014/01/02 15:05:32, benji1 wrote:
> Unfortunately the non-256 color terminal colors are not standardized
so I
> suggest testing on multiple terminals (I can test on xterm, gnome
terminal, and
> terminator if you want).

My understanding is that these color combinations can be safely used
cross-terminal because the colors are handled by Urwid itself:
http://excess.org/urwid/docs/reference/constants.html?highlight=colors#foreground-and-background-colors
But I can be wrong, and maybe it is worth testing it.

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

https://codereview.appspot.com/44750044/diff/1/quickstart/cli/views.py#newcode168
quickstart/cli/views.py:168: for position, env_data in
enumerate(environments):
On 2014/01/02 15:19:28, gary.poster wrote:
> Just as a way to make the main function smaller, what would you think
of
> breaking this block out into its own function?

> ...eh, nevermind. I tried to do it and you need default_found and
> focus_position and errors_found, so it doesn't seem like there's much
point.

> The function is very clear and linear but I was hoping to make it
shorter by
> making some nice internal verbs. Maybe you have another idea? Could
be for
> later, or never. :-)

Not sure, a quick solution might be splitting the function into three
parts (existing envs selection, new env creation, status management) in
a way similar to what you suggested. It could be worth doing that,
especially if we find in the future that the code starts ti be too much
nested. For now, as you mentioned, the function is quite flat and easy
to follow.

https://codereview.appspot.com/44750044/diff/1/quickstart/cli/views.py#newcode230
quickstart/cli/views.py:230: def use(env_data):
On 2014/01/02 15:19:28, gary.poster wrote:
> I wonder if the env_detail function would look less forbidding at
first glance
> if these internal functions were defined outside (_use, _set_default,
etc.) and
> then hooked up as partials. I think the verbs are clear enough that
it would
> read well.

> A counter-argument is that you are defining everything locally, so you
can read
> what is going on inline, and it similar to what people are accustomed
to doing
> with OO. If env_detail were in its own file as is, would it look
super easy,
> because it would look similar to a class with methods? I don't know.

> What do you think?

This is a really good question. I like the possibility to use closures
so that you can share the scope and group code logic, but, as you
mentioned, this can be surprising at first sight. If you add to this the
good point Benji made below (it is easier to directly unit test separate
functions), then we have two good arguments to make this change. Since
it requires a bit of code reorganization I'd be inclined to tackle this
separately, how does it sound?

https://codereview.appspot.com/44750044/diff/1/quickstart/cli/views.py#newcode347
quickstart/cli/views.py:347: def save(env_data, get_new_env_data):
On 2014/01/02 15:05:32, benji1 wrote:
> I like using functions defined in functions like this, but it means
that they
> are hard to test (or require functional tests instead of unit tests).
Consider
> making them module-level functions so they can be tested directly.

Very good point. Please see my answer to a similar comment from Gary
above.

https://codereview.appspot.com/44750044/diff/1/quickstart/tests/cli/test_views.py
File quickstart/tests/cli/test_views.py (right):

https://codereview.appspot.com/44750044/diff/1/quickstart/tests/cli/test_views.py#newcode166
quickstart/tests/cli/test_views.py:166:
@mock.patch('quickstart.cli.views.env_detail')
On 2014/01/02 15:19:28, gary.poster wrote:
> This sort of thing makes me think we ought to pass the needed env_*
around as
> arguments everywhere. :-)

Do you mean, as kwargs? e.g. def env_index(app, ...,
env_detail=env_detail)?
That would avoid the need to patch the function here: it would be
possible to just pass a mock object as the kwarg.

https://codereview.appspot.com/44750044/