Next bit I looked at:
=== added file 'lib/lp/archivepublisher/scripts/generate_extra_overrides.py'
+ def processOptions(self):
+ """Handle command-line options."""
+ if self.options.distribution is None:
+ raise OptionValueError("Specify a distribution.")
+
+ self.distribution = getUtility(IDistributionSet).getByName(
+ self.options.distribution)
+ if self.distribution is None:
+ raise OptionValueError(
+ "Distribution '%s' not found." % self.options.distribution)
+
+ series = None
+ wanted_status = (SeriesStatus.DEVELOPMENT,
+ SeriesStatus.FROZEN)
+ for status in wanted_status:
+ series = self.distribution.getSeriesByStatus(status)
+ if series.count() > 0:
+ break
+ else:
+ raise LaunchpadScriptFailure(
+ 'There is no DEVELOPMENT distroseries for %s' %
+ self.options.distribution)
+ self.series = series[0]
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?
The structure of that loop is also a bit hard to follow. It gets easier if you
have a single-purpose function that you can just return from instead of using a
break!
Finally, a few cosmetic tips:
* When line-breaking a tuple, list, or dict, our “house style” is to add a
line break right after the opening parenthesis, bracket, or brace. Each entry
ends in a comma and a newline — including the last entry.
wanted_status = (
SeriesStatus.DEVELOPMENT,
SeriesStatus.FROZEN,
)
* You don't really need series.count(). In fact I think you can just retrieve
the first matching series as series.first(), and you'll get None if there isn't
any.
* 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.
--
https://code.launchpad.net/~cjwatson/launchpad/refactor-cron-germinate/+merge/84624
Your team Launchpad code reviewers is requested to review the proposed merge of
lp:~cjwatson/launchpad/refactor-cron-germinate into lp:launchpad.
_______________________________________________
Mailing list: https://launchpad.net/~launchpad-reviewers
Post to : [email protected]
Unsubscribe : https://launchpad.net/~launchpad-reviewers
More help : https://help.launchpad.net/ListHelp
