Compute documentation statistics.

Compute documentation statistics.

Count the total number of YUIDoc documentation lines by
enhancing the output of the "bin/lint-yuidoc" command.

https://code.launchpad.net/~teknico/juju-gui/extract-doc-stats/+merge/135958

(do not edit description out of merge proposal)

[teknico, 2012-11-23 18:04:20]

Please take a look.

[benji, 2012-11-23 18:42:17]

Thanks for the branch.  The only comment that I made that requires action before
landing the branch is tweaking the docstrings.

I had hoped that the next time we touched this code we would add tests.  Maybe
next time.

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

https://codereview.appspot.com/6845085/diff/1/bin/lint-yuidoc#newcode3
bin/lint-yuidoc:3: """
Docstrings should not start with a blank line (i.e., the opening triple-quote
should not be on a line by itself).

See PEP-257 for docstring a style guide.

This applies to all the docstrings in this branch.

https://codereview.appspot.com/6845085/diff/1/bin/lint-yuidoc#newcode90
bin/lint-yuidoc:90: in_undocumented = (file_name, function_name) in undocumented
I can't say I am keen on some of the variable name changes, especially this one,
but I guess that's a taste thing.

https://codereview.appspot.com/6845085/diff/1/bin/lint-yuidoc#newcode103
bin/lint-yuidoc:103: return (
This branch appears to have two different line-continuation styles involving
parenthesis.  The one here (opening paren on a line by itself) and on line 131
of this diff (opening parent plus as many items as will fit on the first line).

One or the other would be preferable.

https://codereview.appspot.com/6845085/diff/1/bin/lint-yuidoc#newcode126
bin/lint-yuidoc:126: except ValueError:
Good fix.

https://codereview.appspot.com/6845085/diff/1/bin/lint-yuidoc#newcode131
bin/lint-yuidoc:131: (file_functions, missing_doc_functions,
falsely_undoc_functions,
Since this function only deals with functions, adding "functions" and "file" to
these variable names does not add much.  That, or I'm in an overly persnickety
mood.

[teknico, 2012-11-26 09:29:12]

benji wrote:
> Thanks for the branch.  The only comment that I made that requires action
before
> landing the branch is tweaking the docstrings.

True, sorry, I was a bit lazy with those docstrings, I'll fix them.

> I had hoped that the next time we touched this code we would add tests.  Maybe
> next time.

I was wondering about that too. I'm happy to write them, not sure where to put
them though. My preference would be to move the script code to a lib/scripts/
directory and then import it from a launching stub in bin/, so that the code is
testable. But the lib/ directory contains Javascript code and is not a Python
package. Any advice?
 
> I can't say I am keen on some of the variable name changes, especially this
one,
> but I guess that's a taste thing.

Uhm, ok, I'll try to undo them as much as I can. :-)

> This branch appears to have two different line-continuation styles involving
> parenthesis.  The one here (opening paren on a line by itself) and on line 131
> of this diff (opening parent plus as many items as will fit on the first
line).
> 
> One or the other would be preferable.

Of course, I'll clean that up too.

> Since this function only deals with functions, adding "functions" and "file"
to
> these variable names does not add much.  That, or I'm in an overly persnickety
> mood.

Well, I like the code to depend on the surrounding context as little as it can,
and use the same names for the same values in different contexts. It's a
tradeoff between clarity and long-windedness, I guess.

Thanks for this review and for any additional advice, Benji.

[benji, 2012-11-26 13:40:40]

On Mon, Nov 26, 2012 at 9:29 AM,  <nicola.larosa@canonical.com> wrote:
> benji wrote:
>>
>> Thanks for the branch.  The only comment that I made that requires
>
> action before
>>
>> landing the branch is tweaking the docstrings.
>
>
> True, sorry, I was a bit lazy with those docstrings, I'll fix them.

Thanks.

Perhaps I am missing something, but I don't see your comments in
Reitveld.  Did you respond there or just reply to the email?

For most of the reviews I have done people reply via Reitveld.  That
seems like the best route, but I am not sure if others prefer one over
the other.

>> I had hoped that the next time we touched this code we would add
>
> tests.  Maybe
>>
>> next time.
>
>
> I was wondering about that too. I'm happy to write them, not sure where
> to put them though. My preference would be to move the script code to a
> lib/scripts/ directory and then import it from a launching stub in bin/,
> so that the code is testable. But the lib/ directory contains Javascript
> code and is not a Python package. Any advice?

If we were using Buildout it would be easy.  Barring that, your approach
sounds good to me.  We don't need any packages, we can just have the
shim script in bin/ invoke the script in lib/scripts and have the test
module live in lib/scripts too.

One downside of this approach is that the current working directory will
have to be the root of the checkout for the shim script to be able to
find the "real" script.  (Unless we do something fancy in the shim
script, which does not seem to be worth the effort.)

>> I can't say I am keen on some of the variable name changes, especially
>
> this one,
>>
>> but I guess that's a taste thing.
>
>
> Uhm, ok, I'll try to undo them as much as I can. :-)

I appreciate it, but I adhere to the "tie goes to the runner" rule.  If
you really like these names better, then that is fine.  If my comment
had made you think "Oh, yeah, I don't like them either" then that would
have been another story.

>> This branch appears to have two different line-continuation styles
>
> involving
>>
>> parenthesis.  The one here (opening paren on a line by itself) and on
>
> line 131
>>
>> of this diff (opening parent plus as many items as will fit on the
>
> first line).
>
>> One or the other would be preferable.
>
>
> Of course, I'll clean that up too.

Cool.

>> Since this function only deals with functions, adding "functions" and
>
> "file" to
>>
>> these variable names does not add much.  That, or I'm in an overly
>
> persnickety
>>
>> mood.
>
>
> Well, I like the code to depend on the surrounding context as little as
> it can, and use the same names for the same values in different
> contexts. It's a tradeoff between clarity and long-windedness, I guess.

I am definitely on board with the "same names for the same values in
different contexts."
-- 
Benji York

[gary.poster, 2012-11-26 14:24:50]

Hi Nicola.  This is a nice improvement.  Thank you!

I agree that fixing the docstring formatting would be a good idea.  Otherwise,
in some cases I gently agree with Benji and in others I prefer what you've
done--in particular, I like the name changes to "file_functions,
missing_doc_functions,
falsely_undoc_functions".

Gary

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

https://codereview.appspot.com/6845085/diff/1/bin/lint-yuidoc#newcode90
bin/lint-yuidoc:90: in_undocumented = (file_name, function_name) in undocumented
On 2012/11/23 18:42:17, benji wrote:
> I can't say I am keen on some of the variable name changes, especially this
one,
> but I guess that's a taste thing.
I see where Benji is coming from, but it does read nicely to me in line 95
below--it seems slightly more explicit.  I guess I would lean Benji's way for
this one, but am fine with Nicola's changes also.

I think Nicola's other changes so far that I've seen in this file are nice
improvements.

https://codereview.appspot.com/6845085/diff/1/bin/lint-yuidoc#newcode131
bin/lint-yuidoc:131: (file_functions, missing_doc_functions,
falsely_undoc_functions,
On 2012/11/23 18:42:17, benji wrote:
> Since this function only deals with functions, adding "functions" and "file"
to
> these variable names does not add much.  That, or I'm in an overly persnickety
> mood.
As someone largely unfamiliar with this code, I find the changes helpful.

[teknico, 2012-11-26 15:43:11]

*** Submitted:

Compute documentation statistics.

Count the total number of YUIDoc documentation lines by
enhancing the output of the "bin/lint-yuidoc" command.

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

[teknico, 2012-11-26 17:23:11]

benji wrote:
> Perhaps I am missing something, but I don't see your comments
> in Reitveld.  Did you respond there or just reply to the email?

Weird. I replied (and am replying again) on Rietveld, and can see my previous
reply here.

> If we were using Buildout it would be easy.  Barring that, your
> approach sounds good to me.  We don't need any packages, we can
> just have the shim script in bin/ invoke the script in
> lib/scripts and have the test module live in lib/scripts too.

Do you mean calling it as an external command? I'd rather import code in
lib/scripts/ from the shim script in bin/.

> One downside of this approach is that the current working
> directory will have to be the root of the checkout for the shim
> script to be able to find the "real" script.  (Unless we do
> something fancy in the shim script, which does not seem to be
> worth the effort.)

Well, nothing *very* fancy, I guess, just the usual path building based on
os.path.dirname(__file__).

> I appreciate it, but I adhere to the "tie goes to the runner" rule.
> If you really like these names better, then that is fine.  If my
> comment had made you think "Oh, yeah, I don't like them either"
> then that would have been another story.

Ok, since Gary has also said he likes the "file_functions" name and the
"_functions" suffixes, I left them in. I did revert "in_undocumented" to
"is_documented", though.