yellow team mailing list archive

[Nicola Larosa <nicola.larosa@xxxxxxxxxxxxx>]

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.


https://codereview.appspot.com/6845085/

-- 
https://code.launchpad.net/~teknico/juju-gui/extract-doc-stats/+merge/135958
Your team Juju GUI Hackers is requested to review the proposed merge of lp:~teknico/juju-gui/extract-doc-stats into lp:juju-gui.