Merge lp:~stevenk/launchpad/db-add-derivedistroseries-api into lp:launchpad/db-devel

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.

Hi Graham,

The reason I'm checking for those arguments is because the API call can be used one of two ways. Firstly, it can create the distroseries for the caller, and secondly, it can use the distroseries that already exists, so we only need to check if arguments are set if we are creating a distroseries.

On 12 October 2010 11:43, Steve Kowalik <email address hidden> wrote:
> Hi Graham,
>
> The reason I'm checking for those arguments is because the API call can be used one of two ways. Firstly, it can create the distroseries for the caller, and secondly, it can use the distroseries that already exists, so we only need to check if arguments are set if we are creating a distroseries.

Okay, I see your reasoning. However, I think that the code still needs
clarification. I'd rather see:

  if not displayname:
      raise DerivationError(...)
  if not title:
      raise DerivationError(...)
...

Etc. That way it's clearer upon reading what's going on.

Final nitpick, otherwise r=me:

> 210 + if displayname is None or len(displayname) == 0:

Why not just use `if not displayname`? (Lather, rinse, repeat for other conditions in this method).

> 211 + raise DerivationError("Display Name needs to be set when"
> 212 + " creating a distroseries.")

When you have to wrap a call, always start wrapping after the opening parenthesis:

    raise DerivationError(
        "Display Name needs to be set when"
        " creating a distroseries.")

(And if the string will fit on one line thereafter, make it so.)