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. -- Mailing list: https://launchpad.net/~yellow Post to : [email protected] Unsubscribe : https://launchpad.net/~yellow More help : https://help.launchpad.net/ListHelp
