Add yuidoc linter.

Add yuidoc linter.

In an attempt to not introduce reams of lint straight off, there is a file
named "undocumented" that includes the currently undocumented functions.  Once
a function is documented it should be removed from that file (and the script
will complain if it is not).

https://code.launchpad.net/~benji/juju-gui/doc-linter/+merge/131833

(do not edit description out of merge proposal)

[benji, 2012-10-29 09:07:53]

Please take a look.

[gary.poster, 2012-10-29 10:21:09]

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?

[benji.york, 2012-10-29 11:20:44]

Thanks for the great review.  I have addressed each of your comments and pushed
the changes.

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):
On 2012/10/29 10:21:09, gary.poster wrote:
> 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...

Done.

https://codereview.appspot.com/6820047/diff/1/Makefile#newcode39
Makefile:39: @bin/lint-yuidoc
On 2012/10/29 10:21:09, gary.poster wrote:
> It makes me giggle slightly to have one branch disagree with itself

:)  Yeah the noun/verb split was why I did that.  I tend toward make
targets being nouns ("make docs", etc.) and command names being verbs
(like "make").

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.
On 2012/10/29 10:21:09, gary.poster wrote:
> No.  'If a value is not provided, "cs" is assumed.'
> 
> If a value is provided, it is honored.

Ah!  I wondered why this function was insane.  It turns out that it was me. 
Fixed.

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
On 2012/10/29 10:21:09, gary.poster wrote:
> 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*\(')
On 2012/10/29 10:21:09, gary.poster wrote:
> 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.

Comments added.  Thanks.

> Why the + on "[:=]+"?  That doesn't make sense to me: I can only see why you
> would have one of those, once.

Good catch.  Fixed.

> 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?

Another good catch.  I have expanded the regex and tweaked the Python to
pull the function name from either the assignment (as it was before) or
the explicitly provided name.

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

k

https://codereview.appspot.com/6820047/diff/1/bin/lint-yuidoc#newcode27
bin/lint-yuidoc:27: # If we find a documentation block, return its location.
On 2012/10/29 10:21:09, gary.poster wrote:
> This comment is false.  We return a bool.

Yep.  It was the line number previously but the comment did not get updated. 
Thanks and fixed.

https://codereview.appspot.com/6820047/diff/1/bin/lint-yuidoc#newcode43
bin/lint-yuidoc:43: for function_name, line_number in find_functions(source):
On 2012/10/29 10:21:09, gary.poster wrote:
> It strikes me that we could probably do this cleanly in a single pass

You might be right.  I also agree with your reasons to just leave it for
now.  If I/we touch this code much more I would like us to add a good
set of unit tests first.  That will probably inform the design quite a
bit.

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
On 2012/10/29 10:21:09, gary.poster wrote:
> typo: function

Done.

https://codereview.appspot.com/6820047/diff/1/bin/lint-yuidoc#newcode66
bin/lint-yuidoc:66: def missing_undocumented(functions, undocumented):
On 2012/10/29 10:21:09, gary.poster wrote:
> 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.

Done.

https://codereview.appspot.com/6820047/diff/1/bin/lint-yuidoc#newcode88
bin/lint-yuidoc:88: found_errors = True
On 2012/10/29 10:21:09, gary.poster wrote:
> 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.

I like it.  I might have been too verbose.  We can fix that later if so.

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
You are right.  The problem was that we're not really parsing the JS.  I
slightly complexified the heuristic to fix it.  If we have to do much more to it
I would recommend switching to using the output of yuidoc (yuidoc/data.json) to
decide if a function is documented or not.

[benji, 2012-10-29 11:21:02]

Please take a look.

[gary.poster, 2012-10-29 11:47:05]

Looks good, Benji. Thanks again.

Gary

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

https://codereview.appspot.com/6820047/diff/7001/bin/lint-yuidoc#newcode13
bin/lint-yuidoc:13: '^ *(?:(\w+) *[:=] *function *|function +(\w+)) *\(')
Cool, thank you. looks good

https://codereview.appspot.com/6820047/diff/7001/bin/lint-yuidoc#newcode79
bin/lint-yuidoc:79: def remove(unwanted, seq):
that's a nicer function :-)

https://codereview.appspot.com/6820047/diff/7001/bin/lint-yuidoc#newcode118
bin/lint-yuidoc:118: print 'entries therein.  The people of the future thank
you.'
Any message from the People of the Future (did you consider that
capitalization?) has my support.

[benji, 2012-10-29 14:27:00]

*** Submitted:

Add yuidoc linter.

In an attempt to not introduce reams of lint straight off, there is a file
named "undocumented" that includes the currently undocumented functions.  Once
a function is documented it should be removed from that file (and the script
will complain if it is not).

R=gary.poster, benji.york
CC=
https://codereview.appspot.com/6820047