yellow team mailing list archive

[Benji York <benji.york@xxxxxxxxxxxxx>]

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.

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.