Code review comment for lp:~stevenk/launchpad/db-add-derivedistroseries-api

Hi Steve,

Overall I'm happy with this branch, but there are a couple of fixes that
it needs before it's ready to land:

> 195 + def deriveDistroSeries(
> 196 + self, user, name, distribution=None, displayname=None,
> 197 + title=None, summary=None, description=None, version=None,
> 198 + status=SeriesStatus.FROZEN, architectures=(), packagesets=(),
> 199 + rebuild=False):

Minor nitpick: When defining methods (as opposed to calling them) we
wrap their arguments thus so that the difference between method
declaration and code is clearer:

    def deriveDistroSeries(self, user, name, distribution=None,
                           displayname=None, title=None, summary=None,
                           description=None, version=None,
                           status=SeriesStatus.FROZEN, architectures=(),
                           packagesets=(), rebuild=False):

> 210 + for param in (
> 211 + displayname, title, summary, description, version):
> 212 + if param is None or len(param) == 0:
> 213 + raise DerivationError(
> 214 + "Display Name, Title, Summary, Description and"
> 215 + " Version all need to be set when creating a"
> 216 + " distroseries")

Why bother to do this at all? Why not just put all of these things
earlier in the parameter list and make them required rather than
optional? This validation code is quite confusing; it took me > 5
seconds to work out what was going on and if a DerivationError gets
raised it doesn't actually tell you which parameter you forgot to pass.
Might as well let Python do the work for you.

> 351 + login('<email address hidden>')

This (and all subsequent lines that do the same thing) should use the
ADMIN_EMAIL constant from lp.testing.sampledata rather than having the
email address hardcoded.