Code review - Issue 6845085: Compute documentation statistics.https://codereview.appspot.com/2012-11-26T17:23:11+00:00rietveld
Message from unknown
2012-11-23T18:04:16+00:00teknicourn:md5:b268066ad0970c06530ff8c32b527d3c
Message from nicola.larosa@canonical.com
2012-11-23T18:04:20+00:00teknicourn:md5:7c4c3424f65e17f389644fcc97658622
Please take a look.
Message from benji.york@gmail.com
2012-11-23T18:42:17+00:00benjiurn:md5:b0b7c1596c790184c3fa96c89867f45b
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.
Message from nicola.larosa@canonical.com
2012-11-26T09:29:12+00:00teknicourn:md5:9e51f25ca865c25b1d82f0195e5c8d1b
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.
Message from benji.york@gmail.com
2012-11-26T13:40:40+00:00benjiurn:md5:beba24ce702c841555c768b53df11481
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
Message from gary.poster@canonical.com
2012-11-26T14:24:50+00:00gary.posterurn:md5:ee6c4b82c18fa9cc253416a327664c07
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.
Message from unknown
2012-11-26T15:39:22+00:00teknicourn:md5:0125997592a4ee7a52dc266329db5c8a
Message from nicola.larosa@canonical.com
2012-11-26T15:43:11+00:00teknicourn:md5:446bf0ea60625f14e2a928f94d053d08
*** 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
Message from nicola.larosa@canonical.com
2012-11-26T17:23:11+00:00teknicourn:md5:3723c5f9796cdec31e739c1c62eb92ca
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.