launchpad-reviewers team mailing list archive

[Abel Deuring <abel.deuring@xxxxxxxxxxxxx>]

Review: Approve code

Looks good. Just a formal nitpick:

=== modified file 'lib/lp/soyuz/browser/archive.py'
--- lib/lp/soyuz/browser/archive.py	2012-07-20 10:14:01 +0000
+++ lib/lp/soyuz/browser/archive.py	2012-08-09 11:36:26 +0000
@@ -380,6 +380,8 @@
         if item_type is None or item is None:
             return None
 
+        the_item = None
+        kwargs = {}
         if item_type == 'component':
             # See if "item" is a component name.
             try:
@@ -406,14 +408,21 @@
             # See if "item" is a pocket name.
             try:
                 the_item = PackagePublishingPocket.items[item]
+                # Was a 'series' URL param passed?
+                series = get_url_param('series')
+                if series is not None:
+                    # Get the requested distro series.
+                    try:
+                        series = self.context.distribution[series]
+                        kwargs["distroseries"] = series
+                    except NotFoundError:
+                        pass
             except KeyError:
                 pass
-        else:
-            the_item = None

Here, you remove the "else" part of a longer if/elif/elif. The LP style
guide requires that these constructs always have a final "else" block,
even if it is just an

    else:
        # Nothing to do because...
        pass

The required part in such a case is the comment. I think it would be
more simple here to revert to the original "else: the_item = None".

-- 
https://code.launchpad.net/~cjwatson/launchpad/fix-pocket-queue-admin-series/+merge/118929
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.