Code review comment for lp:~frankban/juju-quickstart/env-manage-edit
Revision history for this message
| Gary Poster (gary) wrote | # |
© 2004
Canonical Ltd.
•
Terms of use
•
Data privacy
•
Contact Launchpad Support
•
Blog
•
Careers
•
System status
•
bbfa235
(Get the code!)
LGTM! Thank you!
https:/
File quickstart/
https:/
quickstart/
error):
Nice behavior/approach for the widget generation. I like it.
https:/
quickstart/
For readability, I wonder if we ought to stick most of this block in a
separate function.
if generate_method is not None:
widgets.
...or similar?
https:/
quickstart/
Hah, ok, cool, that's what I mentioned in my qa. :-)
https:/
quickstart/
errors below')),
Nice to have: a count. ("Please correct the 2 errors below")
https:/
quickstart/
checkboxes.
s/as/are/
Boolean values are represented a checkboxes
https:/
quickstart/
value_getter in form.items())
Nice :-)
https:/
File quickstart/
https:/
quickstart/
enumerate(
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:/
quickstart/
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:/
File quickstart/
https:/
quickstart/
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:/
quickstart/
Maybe use a dict or namedtuple instead? Shrug.
https:/
quickstart/
error, generate, choices, default = (
That works too. :-)
https:/
File quickstart/
https:/
quickstart/
@mock.patch(
This sort of thing makes me think we ought to pass the needed env_*
around as arguments everywhere. :-)
https:/
quickstart/
zip(env_types, buttons[
nice
https:/