LGTM!  Thank you!


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#newcode44
quickstart/cli/forms.py:44: def create_string_widget(field, value,
error):
Nice behavior/approach for the widget generation.  I like it.

https://codereview.appspot.com/44750044/diff/1/quickstart/cli/forms.py#newcode82
quickstart/cli/forms.py:82: if generate_method is not None:
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?

https://codereview.appspot.com/44750044/diff/1/quickstart/cli/forms.py#newcode98
quickstart/cli/forms.py:98: # edit widget when a choice is clicked.
Hah, ok, cool, that's what I mentioned in my qa. :-)

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')),
Nice to have: a count.  ("Please correct the 2 errors below")

https://codereview.appspot.com/44750044/diff/1/quickstart/cli/forms.py#newcode179
quickstart/cli/forms.py:179: # Boolean values as represented as
checkboxes.
s/as/are/

Boolean values are represented a checkboxes

https://codereview.appspot.com/44750044/diff/1/quickstart/cli/forms.py#newcode195
quickstart/cli/forms.py:195: return dict((key, value_getter()) for key,
value_getter in form.items())
Nice :-)

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):
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. :-)

https://codereview.appspot.com/44750044/diff/1/quickstart/cli/views.py#newcode230
quickstart/cli/views.py:230: def use(env_data):
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?

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

https://codereview.appspot.com/44750044/diff/1/quickstart/tests/cli/test_forms.py#newcode1
quickstart/tests/cli/test_forms.py:1: # This file is part of the Juju
Quickstart Plugin, which lets users set up a
On 2014/01/02 15:05:32, benji1 wrote:
> These tests look really nice.  The running comments in each test
really increase
> understanding of what is going on.

+1 on both

https://codereview.appspot.com/44750044/diff/1/quickstart/tests/cli/test_forms.py#newcode80
quickstart/tests/cli/test_forms.py:80: return (
Maybe use a dict or namedtuple instead?  Shrug.

https://codereview.appspot.com/44750044/diff/1/quickstart/tests/cli/test_forms.py#newcode91
quickstart/tests/cli/test_forms.py:91: wrapper, edit, caption, help,
error, generate, choices, default = (
That works too. :-)

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')
This sort of thing makes me think we ought to pass the needed env_*
around as arguments everywhere. :-)

https://codereview.appspot.com/44750044/diff/1/quickstart/tests/cli/test_views.py#newcode200
quickstart/tests/cli/test_views.py:200: for env_type, button in
zip(env_types, buttons[-len(env_types):]):
nice

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