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