Code review comment for lp:~bac/launchpad/bug-487965

Hi Brad,

This is a nice improvement.

merge-conditional

-Edwin

>=== modified file 'lib/lp/registry/browser/product.py'
>--- lib/lp/registry/browser/product.py 2009-11-23 14:52:25 +0000
>+++ lib/lp/registry/browser/product.py 2009-11-25 18:30:38 +0000
>@@ -474,9 +474,9 @@
>
> def _sort_distros(a, b):

Since this function doesn't actually sort a & b, it should be
called _compare_distros.

> """Put Ubuntu first, otherwise in alpha order."""
>- if a['name'] == 'ubuntu':
>+ if a == 'ubuntu':
> return -1

This only works if 'ubuntu' is always passed in as the first
parameter. There needs to be a corresponding if-statement for
b=='ubuntu'.

>- return cmp(a['name'], b['name'])
>+ return cmp(a, b)
>
>
> class ProductSetBreadcrumb(Breadcrumb):
>@@ -947,29 +947,33 @@
> title, and an attribute "packagings" which is a list of the relevant
> packagings for this distro and product.
> """
>- distros = {}
>- # first get a list of all relevant packagings
>+ # First get a list of all relevant packagings.
> all_packagings = []
> for series in self.context.series:
> for packaging in series.packagings:
> all_packagings.append(packaging)
>- # we sort it so that the packagings will always be displayed in the
>- # distroseries version, then productseries name order
>+ # We sort it so that the packagings will always be displayed in the
>+ # distroseries version, then productseries name order.
> all_packagings.sort(key=lambda a: (a.distroseries.version,
> a.productseries.name, a.id))
>+
>+ distros = {}
> for packaging in all_packagings:
>- if distros.has_key(packaging.distroseries.distribution.name):
>- distro = distros[packaging.distroseries.distribution.name]
>+ distribution = packaging.distroseries.distribution
>+ if distribution.name in distros:
>+ distro = distros[distribution.name]
> else:
>- distro = {}
>- distro['distribution'] = packaging.distroseries.distribution
>- distro['packagings'] = []
>+ # Create a dictionary for the distribution.
>+ distro = dict(
>+ distribution=packaging.distroseries.distribution,

Since you created the new distribution variable, this could be shortened to:
  distribution=distribution

>+ packagings=[])
> distros[packaging.distroseries.distribution.name] = distro

The index could be shortened to just distribution.name.
Alternatively, the entire if-else-statement could be replaced with:
    distro = distros.setdefault(
        distribution.name, dict(distribution=distribution, packaging=[]))
but that is up to you, since you might find it less readable.

> distro['packagings'].append(packaging)
>- # now we sort the resulting set of "distro" objects, and return that
>- result = distros.values()
>- result.sort(cmp=_sort_distros)
>- return result
>+ # Now we sort the resulting list of "distro" objects, and return that.
>+ distro_names = distros.keys()
>+ distro_names.sort(cmp=_sort_distros)
>+ results = [distros[name] for name in distro_names]
>+ return results
>
>
> class ProductDownloadFilesView(LaunchpadView,
>