Code review comment for lp:~benji/juju-gui/no-foot-shooting
Revision history for this message
| Gary Poster (gary) wrote | # |
© 2004
Canonical Ltd.
•
Terms of use
•
Data privacy
•
Contact Launchpad Support
•
Blog
•
Careers
•
System status
•
73416ba
(Get the code!)
Good stuff, Benji, thank you.
Land with changes. What I'm bringing is usually trivial, and should be
fairly easy to address, I hope, even when I think it is important.
Gary
https:/
File app/templates/
https:/
app/templates/
serviceIsJujuGUI}}
My inclination is to let the GUI increase the unit count, but not be
exposed--IOW, move serviceIsJujuGUI into only be around
"control-expose," below. Given the basic approach that I think we have
now of "innocent until proven guilty" for disabling GUI service
functionality, do you have a concrete reason to exclude unit count--a
reason to say that this is a problem? Note that I am pretty sure that
unit count may not be reduced beneath one, thanks to a pre-existing
constraint in the GUI.
https:/
File app/templates/
https:/
app/templates/
If you agree that we should reinstate unit count, then IIUC you would
also remove this conditional.
https:/
File app/views/
https:/
app/views/
currently using.
Suggest you add another comment similar to the following:
// Note that the destroy button is not shown for the Juju GUI, so this
is a fail-safe.
https:/
app/views/
this method also
typo: separation
https:/
app/views/
Interesting approach. I like it so far.
https:/
File app/views/
https:/
app/views/
are currently using.
I'm not sure yet from the code, but I'm hoping that this is again a
fail-safe because the "destroy" option is not given to the user, in
which case a comment to that effect would again be my preference.
https:/
app/views/
view.get(
nicer variable name, thank you :-)
https:/
app/views/
menu.one(
Per my previous comment, yep, I see you disable it visually as well.
Cool.
https:/
File app/views/utils.js (right):
https:/
app/views/
(/^(?:[
This is the old regex. It has problems with unofficial charms. I
suggest using the regex from
https:/
the associated tests in
https:/
: "should recognize unofficial charm store URLs" and "should ignore
owners of unofficial charm store URLs". Even if you want to reimplement
yourself for some reason, this is an important case to cover that
affects us today.
https:/
app/views/
candidate.
In the previous branch, Nicola and I both found this to be too fragile.
See the version in
https:/
don't need to use that approach, and *maybe* it is not necessary now for
some reason, but it was necessary in the previous branch for tests to
pass.
https:/
File lib/views/
https:/
lib/views/
This is the kind of thing I wish we didn't do--but I did immediately
above, with the "error" class! In some perfect world, our app would
have enough logical consistency to its construction (and in our
understanding of it) that we could do one of these sorts of things once.
All that said, unless you are sure that this is some new generic CSS
functionality that you are providing, or the warning message is really
rendered in the body of the page rather than within a tighter conceptual
region as defined by a CSS class, or the warning message is used in two
or more very different contexts, I suggest that you consider whether
there is a pertinent section in the LESS to place this, so someone
looking at the general functionality in the LESS file can also see this
part of it.
https:/
File test/test_
https:/
test/test_
I agree that there is a messiness here. If you want to call it out as
needing a fix, please make a bug for it.
To state the obvious, the lie could be addressed by changing the
describe line to say "juju service views" (plural). I expect the more
interesting part to you is how we can divide things up to be more
maintainable.
https:/
test/test_
I don't understand connection. You are using the normal service view,
yeah? We would keep this one in the existing test suite, and only move
out relation views. Suggest deleting comment.
https:/
test/test_
This comment makes sense to me. I suggest being more
specific (e.g., "This would ideally be in its own suite: see XXX at top
of file").
https:/
test/test_
Suggest deletion as before.
https:/
test/test_
Suggest improved comment as before.
https:/
File test/test_
https:/
test/test_
service-
Interesting approach--testing the template and the data gathering and
ignoring the actual rendering hookup itself. I like it, though I must
admit that I miss the fact that this keeps us from testing that the
rendering actually uses the right template. Perhaps if we used the same
render method everywhere, and simply tested whether the class specified
the expected template was in the attribute that the standard render
method used? Please do bring it up Friday!
https:/
test/test_
-1);
I get confused about this but I think we are using mocha plus chai. If
so, maybe this works?
assert.
alternate spelling, which would work nicely with my next comment, would
be this.
expect(
Would be nice if at least one of those worked. :-)
https:/
test/test_
-1);
Could be
expect(
I don't think there is an assert spelling for this.
I usually am not a big fan of the "expect" syntax, but in this case I
think it is more readable than the -1 thing (which I have used myself).
https:/
File test/test_
https:/
test/test_
xxx?
https:/
File test/test_utils.js (right):
https:/
test/test_
As noted before, I have more tests that I suggest you include from lines
428-434 of
https:/
https:/