Code review comment for lp:~cjwatson/launchpad/refactor-cron-germinate

Revision history for this message
Colin Watson (cjwatson) wrote :

On Wed, Dec 07, 2011 at 07:20:29AM -0000, Jeroen T. Vermeulen wrote:
> The part from “series = None” onward seems to be an isolated unit of
> work. I think it looks for the first distroseries in development
> state, or failing that, the first in frozen state. But the structure
> of the code makes that hard to figure out, and then only afterwards I
> can start wondering why you do it exactly this way.
>
> I fervently recommend extracting that code into a sensibly-named
> function. (It doesn't need to be a method: AFAICS a distribution goes
> in, a series comes out, and that is the full extent of its interaction
> with the rest of the world). Come to think of it, might there already
> be a suitable method in Distribution somewhere?

Fair comment. Distribution.currentseries is nearly there: it just
sometimes returns series in statuses we don't want here; but if there's
one we can use then it will always return it, so we can just call
currentseries and then check the result.

> * For Python strings I find consistent double quotes a good habit, at
> least for free-form texts that may just as well contain an
> apostrophe.

This is an out-of-context habit of mine from Perl and shell programming:
since single and double quotes have different interpolation
characteristics there, I've trained myself to use single quotes unless
either I have an explicit reason to want interpolation or the string
contains an apostrophe and no double quotes. I realise this doesn't
make as much sense for Python, so I'll go through and amend this.

« Back to merge proposal