Launchpad itself

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

flags-gui
Merge into devel

Revision history for this message

Martin Pool (mbp) wrote on 2010-09-28:

> 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.

>> 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.

> A pilot error would lead to an oops in the best case or lots of inconvenienced users in the worst case. What if they accidentally enter one field too many, for instance? What of duplicate lines? What of near-duplicate lines with different values? Even if the data is meant to be entered only by careful and usually sober admins, I'd like to see a bit more checking and a lot more testing!

If we look at the various species of pilot error:

0- syntactically invalid, for instance having not enough fields or conflicting priorities: you'll get an oops; it would be much nicer to give you a clean message explaining where you went wrong. however the change won't be committed to the database and it has no lasting impact.

1 - syntactically valid, but not what you meant to say: will be committed to the database and may cause arbitrarily weird things to happen to users.

To me 1 is more important to guard against for obvious reasons.

To reduce the chance of that happening there are various things we can do:
* show a diff of the changes before they're applied
* have a smarter editor that knows what options are available and what values they can take
* allow reverting changes
* store and show a history of changes

I think fixing 1 will largely obsolete 0 because we'll go away from just having a plaintext editor; so I'm not so interested in fixing it by itself. (This is, I admit, also an argument from impatience.)

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.

> 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.

>> 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.

> A pilot error would lead to an oops in the best case or lots of inconvenienced users in the worst case.  What if they accidentally enter one field too many, for instance?  What of duplicate lines?  What of near-duplicate lines with different values? Even if the data is meant to be entered only by careful and usually sober admins, I'd like to see a bit more checking and a lot more testing!

If we look at the various species of pilot error:

0- syntactically invalid, for instance having not enough fields or conflicting priorities: you'll get an oops; it would be much nicer to give you a clean message explaining where you went wrong.  however the change won't be committed to the database and it has no lasting impact.

1 - syntactically valid, but not what you meant to say: will be committed to the database and may cause arbitrarily weird things to happen to users.

To me 1 is more important to guard against for obvious reasons.

To reduce the chance of that happening there are various things we can do:
 * show a diff of the changes before they're applied
 * have a smarter editor that knows what options are available and what values they can take
 * allow reverting changes
 * store and show a history of changes

I think fixing 1 will largely obsolete 0 because we'll go away from just having a plaintext editor; so I'm not so interested in fixing it by itself.  (This is, I admit, also an argument from impatience.)

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.

« Back to merge proposal