launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

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.