yellow team mailing list archive

[Gary Poster <gary.poster@xxxxxxxxxxxxx>]

Thank you for the review, Benji.  I have made the changes that you
requested except for one or two that I discuss below.  Feel free to
reply if you are still concerned about something that I push back on, or
explain/rationalize unsatisfactorily.

Gary


https://codereview.appspot.com/6814089/diff/1/app/templates/charm-description-related.handlebars
File app/templates/charm-description-related.handlebars (right):

https://codereview.appspot.com/6814089/diff/1/app/templates/charm-description-related.handlebars#newcode9
app/templates/charm-description-related.handlebars:9: {{#if
owner}}{{owner}}/{{/if}}{{package_name}}</a>
On 2012/11/06 14:49:55, benji wrote:
> Is the "if" redundant?  I /think/ handlebars will omit the value if it
is null
> or undefined.
Notice the "/" after {{owner}}.

https://codereview.appspot.com/6814089/diff/1/app/views/charm-panel.js
File app/views/charm-panel.js (right):

https://codereview.appspot.com/6814089/diff/1/app/views/charm-panel.js#newcode65
app/views/charm-panel.js:65: /**
On 2012/11/06 14:49:55, benji wrote:
> Our other multi-line yuidoc comments have a row of asterisks down the
left.  Do
> we want to settle on one style or the other?

> My vote would be for asterisks as it helps the comments stand out a
bit.
OK, will do.

https://codereview.appspot.com/6814089/diff/1/app/views/charm-panel.js#newcode71
app/views/charm-panel.js:71: @static
On 2012/11/06 14:49:55, benji wrote:
> I'm curious.  What does "static" mean here?  Is it that this method
doesn't
> access "this"?
Remember I mentioned on a call that yuidoc has no concept of standalone
functions that are not class initializers?  I found three workarounds.
First, you can simply add comments without tags.  I believe I found that
yuidoc or one of our linters was not really happy with this.  It also
seemed annoying to me that you could not specify params and return
values.  Second, Dav Glass said in an email message I found that he
would use tags to specify that this was a class.  I found this hack
deeply unsatisfying. It also produces bad docs IMO, when I tried it.
Third, you can say that this is a method.  Of what?  Good question.
Maybe the window.  But it is a static method, yes: it does not access
"this".

I have a card about this for the weekly retrospective.

https://codereview.appspot.com/6814089/diff/1/app/views/charm-panel.js#newcode80
app/views/charm-panel.js:80: return {
On 2012/11/06 14:49:55, benji wrote:
> This isn't the object literal style we use elsewhere, but I suspect
the normal
> style won't work here because of JavaScript's rules for implicitly
placing
> semicolons.
Correct.  I could wrap it all in parentheses, but that seemed much of a
muchness to me.  What I wrote passes the linter.

https://codereview.appspot.com/6814089/diff/1/app/views/charm-panel.js#newcode87
app/views/charm-panel.js:87: /**
On 2012/11/06 14:49:55, benji wrote:
> I find the Roman dislike for helpful vertical whitespace bothersome.
Could you make a topic about vertical whitespace for a retrospective
discussion?  I wouldn't mind vertical spaces immediately above the
yuidoc comments, separating them from the functions above them.  OTOH, I
find vertical whitespace inserted into function bodies to generally be
distractingly arbitrary if you are not the person who made the
whitespace.  Unfortunately, vertical whitespace are still a stylistic
change about which we can all disagree, despite our beautifiers and
linters.

https://codereview.appspot.com/6814089/diff/1/app/views/charm-panel.js#newcode342
app/views/charm-panel.js:342: @method
On 2012/11/06 14:49:55, benji wrote:
> This directive needs the method name.
Thanks, good catch.

https://codereview.appspot.com/6814089/diff/1/app/views/charm-panel.js#newcode374
app/views/charm-panel.js:374: If there was a failure, render it to the
console and to the
On 2012/11/06 14:49:55, benji wrote:
> This comment and the one just above have different indentation styles.
Yeah, it's in part because the first comment is from outside the hash,
and the second one is within the hash; and possibly also in part because
the beautifier or I are insane, or because we are insane when combined
together.  In any case, I've tried to unify by putting the first comment
withing the hash.

https://codereview.appspot.com/6814089/diff/1/app/views/charm-panel.js#newcode401
app/views/charm-panel.js:401: @method
On 2012/11/06 14:49:55, benji wrote:
> This needs the method name.
Thanks, fixed

https://codereview.appspot.com/6814089/diff/1/app/views/charm-panel.js#newcode431
app/views/charm-panel.js:431: Fires an event indicating that the charm
panel should switch to the
On 2012/11/06 14:49:55, benji wrote:
> <nitpick level="extreme" mode="prose advice from a programmer">I can't
explain
> why exactly, but removing the "s" from "Fires an event" makes this
much better.
> More "active" I guess.</nitpick>
 From a consistency perspective, I do vary between an imperative style
(e.g., "Fire") and a descriptive style (e.g., "Fires").  I tried to
settle on imperative here and elsewhere (e.g. "returns" -> "return").

https://codereview.appspot.com/6814089/diff/1/test/test_charm_panel.js
File test/test_charm_panel.js (right):

https://codereview.appspot.com/6814089/diff/1/test/test_charm_panel.js#newcode278
test/test_charm_panel.js:278: assert(section_container.one('h4
i').hasClass('icon-chevron-up'));
On 2012/11/06 14:49:55, benji wrote:
> Not your fault: The above is rife with brittle selectors.  It would be
great to
> fix this up.
Probably my fault one way or the other. :-P That said, I'm not clear on
what to do that would truly be better.  I could put classes on
everything, but that's fragile in a different dimension IMO.  Since you
don't insist, I'm leaving it.

https://codereview.appspot.com/6814089/

-- 
https://code.launchpad.net/~gary/juju-gui/relatedcharms/+merge/133004
Your team Juju GUI Hackers is requested to review the proposed merge of lp:~gary/juju-gui/relatedcharms into lp:juju-gui.