Additions and changes to docs and comments.

Additions and changes to docs and comments.

Some additions and clarifications to docs and comments,
mainly in app.js, test_app.js, and models/charm.js, including
the promotion to docs of a nice long comment in the latter.

https://code.launchpad.net/~teknico/juju-gui/whats-in-an-app/+merge/145004

(do not edit description out of merge proposal)

[teknico, 2013-01-25 17:56:19]

Please take a look.

[matthew.scott, 2013-01-25 21:56:46]

Excellent docs!  Thank you muchly, land as is.

[gary.poster, 2013-01-26 03:37:28]

Land with changes or as is, your call.

Thank you, Nicola.

Gary

https://codereview.appspot.com/7204057/diff/1/app/app.js
File app/app.js (right):

https://codereview.appspot.com/7204057/diff/1/app/app.js#newcode348
app/app.js:348: // and new ones also, updateEndpoints will replace the whole
map,
You improved this, but it is still unclear to me.  Is this what it means?

// If services have been both added ad removed, "updateEndpoints" will replace
the whole map, so only do this if needed.

I'm not clear from the code that this is in fact the logic, but that's how I
read the prose.

https://codereview.appspot.com/7204057/diff/1/app/app.js#newcode416
app/app.js:416: // Only prefetch once; we redispatch to the service view after
we have
Subtle niggle, but I don't think the semicolon adds value here.  I suggest
replacing it with a period and starting a new sentence.

https://codereview.appspot.com/7204057/diff/1/app/app.js#newcode716
app/app.js:716: *   When more than one route can match a model, the route w/o an
intent is
If you are getting rid of contractions (shrug), I'd guess you want to convert
"w/o" to "without" as well.

https://codereview.appspot.com/7204057/diff/1/app/app.js#newcode735
app/app.js:735: // Fail fast on wildcard paths, routes w/o models,
"without" seems more consistent with your other changes.

For better parallelism, add "on" to the second phrase:

Fail fast on wildcard paths, on routes without models, and when the model...

https://codereview.appspot.com/7204057/diff/1/app/app.js#newcode781
app/app.js:781: // Additionally pass route as options needed to pass through the
I think the intent is as follows.

Additionally pass route as options.  This is needed to pass through the
attribute setter.

https://codereview.appspot.com/7204057/diff/1/app/app.js#newcode794
app/app.js:794: // A whitelist to spare the extra juggling.
"Every comment should be a full sentence" is another rule that can be applied. 
Shrug.

https://codereview.appspot.com/7204057/diff/1/app/app.js#newcode815
app/app.js:815: * Evaluate the request path against all defined routes, and
invoke the
This sounds like you are describing a function.  I suggest that you describe the
data structure and not the function that uses it, or call out the function that
uses this data structure.

https://codereview.appspot.com/7204057/diff/1/app/app.js#newcode870
app/app.js:870: // This alias does not seem to work, including refs by hand.
I am not a huge fan of the "no contractions" rule.  It's alright.  That said, it
again seems to go hand in hand with a "no abbreviations" rule to me, if we are
really going to go for the more formal style.  Therefore I suggest converting
"refs" to "references."

https://codereview.appspot.com/7204057/diff/1/test/test_app.js
File test/test_app.js (right):

https://codereview.appspot.com/7204057/diff/1/test/test_app.js#newcode111
test/test_app.js:111: // 'service/wordpress/' is the primary, so other URLs are
not returned.
suggest "the primary route"

https://codereview.appspot.com/7204057/diff/1/test/test_app.js#newcode237
test/test_app.js:237: // Mock out notifications, so the app can start normally.
Why is "app" ok?  I don't care except for having clear, unsurprising rules.

https://codereview.appspot.com/7204057/diff/1/undocumented
File undocumented (left):

https://codereview.appspot.com/7204057/diff/1/undocumented#oldcode2
undocumented:2: app/app.js:636 "callback"
Thank you :-)

[teknico, 2013-01-28 11:50:34]

Thanks for the suggestions and the oversight catching, Gary.

https://codereview.appspot.com/7204057/diff/1/app/app.js
File app/app.js (right):

https://codereview.appspot.com/7204057/diff/1/app/app.js#newcode348
app/app.js:348: // and new ones also, updateEndpoints will replace the whole
map,
gary.poster wrote:
> You improved this, but it is still unclear to me.
> Is this what it means?

You are right, it is not clear. I rewrote it, and also renamed the related flag.
I hope it is clearer now.

https://codereview.appspot.com/7204057/diff/1/app/app.js#newcode416
app/app.js:416: // Only prefetch once; we redispatch to the service view after
we have
gary.poster wrote:
> Subtle niggle, but I don't think the semicolon adds value here.
> I suggest replacing it with a period
> and starting a new sentence.

It's a matter of personal preference, I guess. I like the semicolon, but it is
not important enough to perform a preference comparative evaluation, so I
changed it as per your suggestion.

https://codereview.appspot.com/7204057/diff/1/app/app.js#newcode716
app/app.js:716: *   When more than one route can match a model, the route w/o an
intent is
gary.poster wrote:
> If you are getting rid of contractions (shrug),
> I'd guess you want to convert "w/o" to "without" as well.

Indeed, done.

https://codereview.appspot.com/7204057/diff/1/app/app.js#newcode735
app/app.js:735: // Fail fast on wildcard paths, routes w/o models,
gary.poster wrote:
> "without" seems more consistent with your other changes.
> 
> For better parallelism, add "on" to the second phrase:
> 
> Fail fast on wildcard paths, on routes without models, and when the model...

Both done.

https://codereview.appspot.com/7204057/diff/1/app/app.js#newcode781
app/app.js:781: // Additionally pass route as options needed to pass through the
gary.poster wrote:
> I think the intent is as follows.
> 
> Additionally pass route as options.  This is needed 
> to pass through the attribute setter.

Nicer, done.

https://codereview.appspot.com/7204057/diff/1/app/app.js#newcode794
app/app.js:794: // A whitelist to spare the extra juggling.
gary.poster wrote:
> "Every comment should be a full sentence" is another rule that can be applied.

> Shrug.

Sure. :-)

https://codereview.appspot.com/7204057/diff/1/app/app.js#newcode815
app/app.js:815: * Evaluate the request path against all defined routes, and
invoke the
gary.poster wrote:
> This sounds like you are describing a function.
> I suggest that you describe the data structure
> and not the function that uses it, or call out
> the function that uses this data structure.

Rephrased, thanks.

https://codereview.appspot.com/7204057/diff/1/app/app.js#newcode870
app/app.js:870: // This alias does not seem to work, including refs by hand.
gary.poster wrote:
> I am not a huge fan of the "no contractions" rule.  It's alright.
> That said, it again seems to go hand in hand with a "no abbreviations" rule to
me,
> if we are really going to go for the more formal style.
> Therefore I suggest converting "refs" to "references."

Indeed, thanks for catching this oversight.

https://codereview.appspot.com/7204057/diff/1/test/test_app.js
File test/test_app.js (right):

https://codereview.appspot.com/7204057/diff/1/test/test_app.js#newcode111
test/test_app.js:111: // 'service/wordpress/' is the primary, so other URLs are
not returned.
gary.poster wrote:
> suggest "the primary route"

Done.

https://codereview.appspot.com/7204057/diff/1/test/test_app.js#newcode237
test/test_app.js:237: // Mock out notifications, so the app can start normally.
gary.poster wrote:
> Why is "app" ok?  I don't care except for having clear, unsurprising rules.

It's not ok, just one more oversight. Done. :-)

[teknico, 2013-01-28 11:52:21]

Please take a look.

[teknico, 2013-01-28 11:58:49]

*** Submitted:

Additions and changes to docs and comments.

Some additions and clarifications to docs and comments,
mainly in app.js, test_app.js, and models/charm.js, including
the promotion to docs of a nice long comment in the latter.

R=matthew.scott, gary.poster
CC=
https://codereview.appspot.com/7204057