yellow team mailing list archive

[Gary Poster <gary.poster@xxxxxxxxxxxxx>]

Hey Benji.  This will be cool to have, thank you.

I have a bunch of rambling comments and suggestions, none of which are
that important, with the possible exception of two.

If I'm right, the more important one is my comment on the "undocumented"
file, last in the line-by-line list below.

The other one is the regex blather I get into in lint-yuidoc.

Let me know what you think.

Thanks

Gary


https://codereview.appspot.com/6820047/diff/1/Makefile
File Makefile (right):

https://codereview.appspot.com/6820047/diff/1/Makefile#newcode2
Makefile:2: # This list can be regenerated with this command (and then
pasted in here):
I misunderstood this comment until I played around with the code.  I
suggest tweaking to something like the following:

After a successful "make" run, this list can be regenerated with this
command...

https://codereview.appspot.com/6820047/diff/1/Makefile#newcode39
Makefile:39: @bin/lint-yuidoc
It makes me giggle slightly to have one branch disagree with itself,
introducing two conflicting spellings.  It's unimportant, but as a
matter of cleanliness it might be nice to settle on one of "yuidoc-lint"
or "lint-yuidoc".  The only argument I could imagine you making for a
difference is about noun-y (yuidoc-lint as make target) vs verb-y
(lint-yuidoc as script name). I think the verb spelling in particular
would work both as make target and script name.

https://codereview.appspot.com/6820047/diff/1/app/models/charm.js
File app/models/charm.js (right):

https://codereview.appspot.com/6820047/diff/1/app/models/charm.js#newcode203
app/models/charm.js:203: * No matter what value is given, "cs" is the
only allowed scheme.
No.  'If a value is not provided, "cs" is assumed.'

If a value is provided, it is honored.

https://codereview.appspot.com/6820047/diff/1/bin/lint-yuidoc
File bin/lint-yuidoc (right):

https://codereview.appspot.com/6820047/diff/1/bin/lint-yuidoc#newcode1
bin/lint-yuidoc:1: #!/usr/bin/env python
Whoa!  What language is this?!

https://codereview.appspot.com/6820047/diff/1/bin/lint-yuidoc#newcode8
bin/lint-yuidoc:8: function_regex = re.compile('^ *([\w]+)\s*[:=]+
*function\s*\(')
Why is it important to require that the string only have space before
the name--that is, why do you have "^ *" at the beginning?  Ah, I have a
guess.  Is that the pattern yuidoc needs in order to work?  I could
imagine this as a real-world function definition...

{ foo: 42, bar: function() { alert('bleh'); } }

...but it might not be supportable by yuidoc.  Yeah?

In any case, either please add a comment explaining the point of this
constraintm or remove it.

Why the + on "[:=]+"?  That doesn't make sense to me: I can only see why
you would have one of those, once.  If there's a reason, it might be
good to comment it, unless I'm being dense for some reason.

This doesn't catch "function [name] (" which actually has been used in
the code.  I *think* we want to include these?  We have two types of
cases of this in the code.  One is the "function as expression" usage of
a name:

app/views/charm.js:23:          'failure': function er(e) {
app/views/charm.js:161:          'failure': function er(e) {
console.error(e.error); }

Naming function expressions in this way only has value if you are using
recursion.  I don't think these examples actually do that, so for
simplicity and uniformity, I'd be in favor of removing those names.
However, we probably should support it in the linter.

Beyond that, we have many function statements, which are of course
required to have a name.  Shouldn't we document them too?

https://codereview.appspot.com/6820047/diff/1/bin/lint-yuidoc#newcode24
bin/lint-yuidoc:24: # documentation, then there is none to be found.
These are interesting heuristics.  It strikes me that we could be more
aggressive, but I suspect that what you have is good enough.

https://codereview.appspot.com/6820047/diff/1/bin/lint-yuidoc#newcode27
bin/lint-yuidoc:27: # If we find a documentation block, return its
location.
This comment is false.  We return a bool.

https://codereview.appspot.com/6820047/diff/1/bin/lint-yuidoc#newcode43
bin/lint-yuidoc:43: for function_name, line_number in
find_functions(source):
It strikes me that we could probably do this cleanly in a single pass
(rather than walking through the file to find functions, and then
walking backwards from each function to discover the presence of
documentation), but I don't care.  Getting this landed soon and moving
on is more valuable, even if I'm right (which I might not be).

https://codereview.appspot.com/6820047/diff/1/bin/lint-yuidoc#newcode58
bin/lint-yuidoc:58: # Otherwise if we found an undocumented funciton
that is not
typo: function

https://codereview.appspot.com/6820047/diff/1/bin/lint-yuidoc#newcode66
bin/lint-yuidoc:66: def missing_undocumented(functions, undocumented):
This is used once and very small, so I don't see the value of factoring
it out of main.  If you agree and feel like doing it, collapse this back
in.

https://codereview.appspot.com/6820047/diff/1/bin/lint-yuidoc#newcode88
bin/lint-yuidoc:88: found_errors = True
Might be nice to also remind the user of the count of undocumented
functions still around (in the "undocumented" file) as a push to improve
things.  As you wish.

https://codereview.appspot.com/6820047/diff/1/undocumented
File undocumented (right):

https://codereview.appspot.com/6820047/diff/1/undocumented#newcode2
undocumented:2: app/app.js getModelURL
uh oh.

This method has a very nice doc string, which you formatted in the
previous branch.  I've never had the opportunity to use this quote more
aptly than I do now: something is rotten in the state of Denmark!

This is not caught by your linter, even given the "complain when
supposedly undocumented things are actually documented" clause, which
suggests the linter has some bugs.  Am I right?

https://codereview.appspot.com/6820047/

-- 
https://code.launchpad.net/~benji/juju-gui/doc-linter/+merge/131833
Your team Juju GUI Hackers is subscribed to branch lp:juju-gui.