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