Code review comment for lp:~mbp/launchpad/flags-gui

On Tue, Sep 28, 2010 at 10:23 PM, Martin Pool <email address hidden> wrote:
>> I don't much care for putting code in __init__.py.  I can see that it keeps the imports shorter, but it's unusual enough that it may be easy to miss.
>
> I realize you said this an a preference not a rule, but I got the idea this was common because there are dozens of existing modules with nontrivial inits.  Perhaps that's no longer prefered.  If so, it should go into https://dev.launchpad.net/PythonStyleGuide.

Putting stuff in __init__ is fine. If its not clear, or the file is
'large', split it out - but thats a normal rule anyhow.

>>> 147     +    def getFeatureRulesURL(self):
>> Needs a docstring.
>
> This is the kind of thing where I question "everything should have a docstring."  It's in test code; it's only used in this class; the name is pretty selfexplanatory and it's only two lines of code.

Agreed; I certainly woudn't put a docstring unless its nonobvious what it is.

> I think the risk is tolerable because the people with access to this already can and do run arbitrary shell and sql commands, and the damage they can do here is strictly less than is possible with them.  I would like to get this just rolling too, and give them something better than raw sql to use.

Also by definition these config values will (at worst) enable a
feature to users that didn't expect it (or conversely disable things
we didn't want disabled [because they needed an explicit-on to work].

Not a huge issue. I like the idea of a diff showing 'this is what
changed', if you care to do it - but I wouldn't block on having it.

-Rob