Merge lp:~wgrant/launchpad/bug-592914-recipe-distroseries-order into lp:launchpad

Proposed by William Grant
Status: Merged
Approved by: Māris Fogels
Approved revision: no longer in the source branch.
Merged at revision: 11016
Proposed branch: lp:~wgrant/launchpad/bug-592914-recipe-distroseries-order
Merge into: lp:launchpad
Diff against target: 50 lines (+9/-5)
2 files modified
lib/lp/code/browser/sourcepackagerecipe.py (+7/-3)
lib/lp/code/browser/tests/test_sourcepackagerecipe.py (+2/-2)
To merge this branch: bzr merge lp:~wgrant/launchpad/bug-592914-recipe-distroseries-order
Reviewer Review Type Date Requested Status
Māris Fogels (community) Approve
Review via email: mp+27415@code.launchpad.net

Commit message

Sort by version the distroseries list when creating recipes and builds.

Description of the change

This branch fixes bug #592914 by sorting the distroseries vocabulary used when creating recipes and requesting builds. It's just reverse sorting by DistroSeries.version, which will fall apart if we ever support multiple distributions, but it will work fine for now.

I've updated the broken test, and there is no lint.

To post a comment you must log in.
Revision history for this message
Māris Fogels (mars) wrote :

Hi William,

The fix looks good, but I had two concerns:

 * Would it be better to use attrgetter instead of the lambda?: sorted(.., key=attrgetter('value.version'))

 * Just a comment, but the call to term.value.version breaks the Law of Demeter (http://en.wikipedia.org/wiki/Law_of_Demeter). That often indicates an object design problem, but since we are working with data objects, I think it can pass here.

Looks good! r=mars

Maris

review: Approve
Revision history for this message
William Grant (wgrant) wrote :

I didn't know that attrgetter could traverse multiple levels like that. I've replaced the lambda with it.

Revision history for this message
William Grant (wgrant) wrote :

It turns out that attrgetter doesn't actually work like that.

Revision history for this message
William Grant (wgrant) wrote :

I've just worked out how it passed locally: python2.6 works as you suggested, but python2.5 only works for a single level.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/code/browser/sourcepackagerecipe.py'
2--- lib/lp/code/browser/sourcepackagerecipe.py 2010-06-15 15:08:11 +0000
3+++ lib/lp/code/browser/sourcepackagerecipe.py 2010-06-16 11:49:28 +0000
4@@ -37,6 +37,7 @@
5 LaunchpadView, Link, Navigation, NavigationMenu, stepthrough)
6 from canonical.launchpad.webapp.authorization import check_permission
7 from canonical.launchpad.webapp.breadcrumb import Breadcrumb
8+from canonical.launchpad.webapp.sorting import sorted_dotted_numbers
9 from canonical.widgets.itemswidgets import LabeledMultiCheckBoxWidget
10 from lp.buildmaster.interfaces.buildbase import BuildStatus
11 from lp.code.errors import ForbiddenInstruction
12@@ -171,9 +172,12 @@
13 ppas = getUtility(IArchiveSet).getPPAsForUser(getUtility(ILaunchBag).user)
14 supported_distros = [ppa.distribution for ppa in ppas]
15 dsset = getUtility(IDistroSeriesSet).search()
16- terms = [SimpleTerm(distro, distro.id, distro.displayname)
17- for distro in dsset if (
18- distro.active and distro.distribution in supported_distros)]
19+ terms = sorted_dotted_numbers(
20+ [SimpleTerm(distro, distro.id, distro.displayname)
21+ for distro in dsset if (
22+ distro.active and distro.distribution in supported_distros)],
23+ key=lambda term: term.value.version)
24+ terms.reverse()
25 return SimpleVocabulary(terms)
26
27 def target_ppas_vocabulary(context):
28
29=== modified file 'lib/lp/code/browser/tests/test_sourcepackagerecipe.py'
30--- lib/lp/code/browser/tests/test_sourcepackagerecipe.py 2010-06-15 15:08:11 +0000
31+++ lib/lp/code/browser/tests/test_sourcepackagerecipe.py 2010-06-16 11:49:28 +0000
32@@ -42,7 +42,7 @@
33 self.ppa = self.factory.makeArchive(
34 displayname='Secret PPA', owner=self.chef, name='ppa')
35 self.squirrel = self.factory.makeDistroSeries(
36- displayname='Secret Squirrel', name='secret',
37+ displayname='Secret Squirrel', name='secret', version='100.04',
38 distribution=self.ppa.distribution)
39 self.squirrel.nominatedarchindep = self.squirrel.newArch(
40 'i386', ProcessorFamily.get(1), False, self.chef,
41@@ -516,8 +516,8 @@
42 Secret PPA (chef/ppa)
43 Distribution series:
44 Secret Squirrel
45+ Hoary
46 Warty
47- Hoary
48 or
49 Cancel""")
50 main_text = self.getMainText(recipe, '+request-builds')