Overall, yay, this branch makes things simpler and more correct. The mixin is still a bit odd, but hopefully noone will have to touch this code for a few years now :-) Just a few comments below, mostly simple clarity stuff. Thanks for fixing the icky bug! Cheers, mwh > === modified file 'lib/lp/code/interfaces/branch.py' > --- lib/lp/code/interfaces/branch.py 2010-04-06 17:19:23 +0000 > +++ lib/lp/code/interfaces/branch.py 2010-04-08 04:14:00 +0000 > @@ -8,7 +8,6 @@ > __metaclass__ = type > > __all__ = [ > - 'bazaar_identity', > 'BRANCH_NAME_VALIDATION_ERROR_MESSAGE', > 'branch_name_validator', > 'BranchCannotBePrivate', > @@ -21,6 +20,7 @@ > 'BranchExists', > 'BranchTargetError', > 'BranchTypeError', > + 'BzrIdentityMixin', > 'CannotDeleteBranch', > 'DEFAULT_BRANCH_STATUS_IN_LISTING', > 'get_blacklisted_hostnames', > @@ -32,12 +32,10 @@ > 'IBranchNavigationMenu', > 'IBranchSet', > 'NoSuchBranch', > - 'UnknownBranchTypeError', > 'user_has_special_branch_access', > ] > > from cgi import escape > -from operator import attrgetter > import re > > from zope.component import getUtility > @@ -69,12 +67,14 @@ > ) > from lp.code.interfaces.branchlookup import IBranchLookup > from lp.code.interfaces.branchtarget import IHasBranchTarget > +from lp.code.interfaces.linkedbranch import ICanHasLinkedBranch > from lp.code.interfaces.hasbranches import IHasMergeProposals > from lp.code.interfaces.hasrecipes import IHasRecipes > from canonical.launchpad.interfaces.launchpad import ( > ILaunchpadCelebrities, IPrivacy) > from lp.registry.interfaces.role import IHasOwner > from lp.registry.interfaces.person import IPerson > +from lp.registry.interfaces.pocket import PackagePublishingPocket > from canonical.launchpad.webapp.interfaces import ( > ITableBatchNavigator, NameLookupFailed) > from canonical.launchpad.webapp.menu import structured > @@ -498,6 +498,14 @@ > "None if not a package branch."), > schema=Interface, required=False, readonly=True)) > > + is_personal_branch = exported( > + Bool( > + title=_("Personal Branch"), required=False, > + readonly=True, default=False, > + description=_( > + "A branch is a personal branch if it is not associated " > + "with a project nor a source package."))) I guess a direct test for this would be nice, although it's so very simple perhaps it's not needed... > code_reviewer = Attribute( > "The reviewer if set, otherwise the owner of the branch.") > > @@ -925,6 +933,17 @@ > def associatedSuiteSourcePackages(): > """Return the suite source packages that this branch is linked to.""" > > + def branchLinks(): > + """Return a sorted list of ICanHasLinkedBranch objects.""" > + > + def branchIdentites(): Missing an 'i' here :) > + """A list of aliases for a branch. > + > + Returns a list of tuples of bzr identity and context object. There is > + at least one alias for any branch, and that is the branch itself. For > + linked branches, the context object is the appropriate linked object. > + """ I'm afraid these two docstrings both need work. The first doesn't explain which ICanHasLinkedBranch objects are returned, nor what being 'sorted' means in this context. The second I think needs to explain itself a bit more as well. It seems to blur the concept of 'alias' (a concept we don't have anywhere else) and the linked object that the alias is derived from. Can you just try to expand it a bit, maybe with an example or two? In this branch it's not really obvious why branchLinks and branchIdentities have a separate existence to the bzr_identity property. I guess this is preparation for that portlet you want to add? I hope if the implementation of that doesn't require all these variants we can shrink them down a bit. > # subscription-related methods > @operation_parameters( > person=Reference( > @@ -1303,50 +1322,48 @@ > """ > > > -def bazaar_identity(branch, is_dev_focus): > - """Return the shortest lp: style branch identity.""" > - lp_prefix = config.codehosting.bzr_lp_prefix > - > - # XXX: TimPenhey 2008-05-06 bug=227602: Since at this stage the launchpad > - # name resolution is not authenticated, we can't resolve series branches > - # that end up pointing to private branches, so don't show short names for > - # the branch if it is private. > - if branch.private: > - return lp_prefix + branch.unique_name > - > - use_series = None > - # XXX: JonathanLange 2009-03-21 spec=package-branches: This should > - # probably delegate to IBranch.target. I would do it now if I could figure > - # what all the optimization code is for. > - if branch.product is not None: > - if is_dev_focus: > - return lp_prefix + branch.product.name > - > - # If there are no associated series, then use the unique name. > - associated_series = sorted( > - branch.associatedProductSeries(), key=attrgetter('datecreated')) > - if len(associated_series) == 0: > - return lp_prefix + branch.unique_name > - # Use the most recently created series. > - use_series = associated_series[-1] > - return "%(prefix)s%(product)s/%(series)s" % { > - 'prefix': lp_prefix, > - 'product': use_series.product.name, > - 'series': use_series.name} > - > - if branch.sourcepackage is not None: > - if is_dev_focus: > - return "%(prefix)s%(distro)s/%(packagename)s" % { > - 'prefix': lp_prefix, > - 'distro': branch.distroseries.distribution.name, > - 'packagename': branch.sourcepackagename.name} > - suite_sourcepackages = branch.associatedSuiteSourcePackages() > - # Take the first link if there is one. > - if len(suite_sourcepackages) > 0: > - suite_source_package = suite_sourcepackages[0] > - return lp_prefix + suite_source_package.path > - > - return lp_prefix + branch.unique_name > +class BzrIdentityMixin: > + """This mixin class determines the bazaar identities. > + > + Used by both the model branch class and the browser branch listing item. > + This allows the browser code to cache the associated links which reduces > + query counts. > + """ > + > + @property > + def bzr_identity(self): > + """See `IBranch`.""" > + identity, context = self.branchIdentities()[0] > + return identity > + > + def branchIdentities(self): > + """See `IBranch`.""" > + lp_prefix = config.codehosting.bzr_lp_prefix > + if self.private or self.is_personal_branch: > + # XXX: thumper 2010-04-08, bug 261609 > + # We have to get around to fixing this > + identities = [] > + else: > + identities = [ > + (lp_prefix + link.bzr_path, link.context) > + for link in self.branchLinks()] > + identities.append((lp_prefix + self.unique_name, self)) > + return identities > + > + def branchLinks(self): > + """See `IBranch`.""" > + links = [] > + for suite_sp in self.associatedSuiteSourcePackages(): > + links.append(ICanHasLinkedBranch(suite_sp)) > + if (suite_sp.distribution.currentseries == suite_sp.distroseries > + and suite_sp.pocket == PackagePublishingPocket.RELEASE): > + links.append(ICanHasLinkedBranch( > + suite_sp.sourcepackage.distribution_sourcepackage)) > + for series in self.associatedProductSeries(): > + links.append(ICanHasLinkedBranch(series)) > + if series.product.development_focus == series: > + links.append(ICanHasLinkedBranch(series.product)) > + return sorted(links) > > > def user_has_special_branch_access(user): > === modified file 'lib/lp/code/model/linkedbranch.py' > --- lib/lp/code/model/linkedbranch.py 2009-08-28 06:39:38 +0000 > +++ lib/lp/code/model/linkedbranch.py 2010-04-08 04:14:00 +0000 > @@ -11,6 +11,9 @@ > from zope.component import adapts > from zope.interface import implements > > +from lazr.enum import EnumeratedType, Item > + > +from lp.archivepublisher.debversion import Version > from lp.code.interfaces.linkedbranch import ICanHasLinkedBranch > from lp.registry.interfaces.distributionsourcepackage import ( > IDistributionSourcePackage) > @@ -21,98 +24,194 @@ > from lp.registry.interfaces.suitesourcepackage import ISuiteSourcePackage > > > -class ProductSeriesLinkedBranch: > +class LinkedBranchOrder(EnumeratedType): > + """An enum used only for ordering.""" > + > + PRODUCT = Item('Product') > + DISTRIBUTION_SOURCE_PACKAGE = Item('Distribution Source Package') > + PRODUCT_SERIES = Item('Product Series') > + SUITE_SOURCE_PACKAGE = Item('Suite Source Package') > + > + > +class BaseLinkedBranch: > + """Provides the common sorting algorithm.""" > + > + def __cmp__(self, other): > + if not ICanHasLinkedBranch.providedBy(other): > + raise AssertionError("Can't compare with: %r" % other) > + return cmp(self.sort_order, other.sort_order) > + > + > +class ProductSeriesLinkedBranch(BaseLinkedBranch): > """Implement a linked branch for a product series.""" > > adapts(IProductSeries) > implements(ICanHasLinkedBranch) > > + sort_order = LinkedBranchOrder.PRODUCT_SERIES > + > def __init__(self, product_series): > - self._product_series = product_series > + self.context = product_series > + > + @property > + def product_series(self): > + return self.context > + > + def __cmp__(self, other): > + result = super(ProductSeriesLinkedBranch, self).__cmp__(other) > + if result != 0: > + return result > + else: > + # When a project gets the series they are ordered alphabetically > + # by name. I don't think this sentence makes sense. I don't suppose it really matters, but sorting series by name isn't how it's usually done in Launchpad; sorting trunk first and then by comparing date_created is more common I think? > + my_parts = ( > + self.product_series.product.name, > + self.product_series.name) > + other_parts = ( > + other.product_series.product.name, > + other.product_series.name) > + return cmp(my_parts, other_parts) > > @property > def branch(self): > """See `ICanHasLinkedBranch`.""" > - return self._product_series.branch > + return self.product_series.branch > > @property > def bzr_path(self): > """See `ICanHasLinkedBranch`.""" > return '/'.join( > - [self._product_series.product.name, self._product_series.name]) > + [self.product_series.product.name, self.product_series.name]) > > def setBranch(self, branch, registrant=None): > """See `ICanHasLinkedBranch`.""" > - self._product_series.branch = branch > - > - > -class ProductLinkedBranch: > + self.product_series.branch = branch > + > + > +class ProductLinkedBranch(BaseLinkedBranch): > """Implement a linked branch for a product.""" > > adapts(IProduct) > implements(ICanHasLinkedBranch) > > + sort_order = LinkedBranchOrder.PRODUCT > + > def __init__(self, product): > - self._product = product > + self.context = product > + > + @property > + def product(self): > + return self.context > + > + def __cmp__(self, other): > + result = super(ProductLinkedBranch, self).__cmp__(other) > + if result != 0: > + return result > + else: > + return cmp(self.product.name, other.product.name) > > @property > def branch(self): > """See `ICanHasLinkedBranch`.""" > - return ICanHasLinkedBranch(self._product.development_focus).branch > + return ICanHasLinkedBranch(self.product.development_focus).branch > > @property > def bzr_path(self): > """See `ICanHasLinkedBranch`.""" > - return self._product.name > + return self.product.name > > def setBranch(self, branch, registrant=None): > """See `ICanHasLinkedBranch`.""" > - ICanHasLinkedBranch(self._product.development_focus).setBranch( > + ICanHasLinkedBranch(self.product.development_focus).setBranch( > branch, registrant) > > > -class PackageLinkedBranch: > +class PackageLinkedBranch(BaseLinkedBranch): > """Implement a linked branch for a source package pocket.""" > > adapts(ISuiteSourcePackage) > implements(ICanHasLinkedBranch) > > + sort_order = LinkedBranchOrder.SUITE_SOURCE_PACKAGE > + > def __init__(self, suite_sourcepackage): > - self._suite_sourcepackage = suite_sourcepackage > + self.context = suite_sourcepackage > + > + @property > + def suite_sourcepackage(self): > + return self.context > + > + def __cmp__(self, other): > + result = super(PackageLinkedBranch, self).__cmp__(other) > + if result != 0: > + return result > + # The versions are reversed as we want the greater Version to sort > + # before the lesser one. Hence self in the other tuple, and other in > + # the self tuple. Next compare the distribution name. > + my_parts = ( > + self.suite_sourcepackage.distribution.name, > + Version(other.suite_sourcepackage.distroseries.version), > + self.suite_sourcepackage.sourcepackagename.name, > + self.suite_sourcepackage.pocket) > + other_parts = ( > + other.suite_sourcepackage.distribution.name, > + Version(self.suite_sourcepackage.distroseries.version), > + other.suite_sourcepackage.sourcepackagename.name, > + other.suite_sourcepackage.pocket) > + return cmp(my_parts, other_parts) > > @property > def branch(self): > """See `ICanHasLinkedBranch`.""" > - package = self._suite_sourcepackage.sourcepackage > - pocket = self._suite_sourcepackage.pocket > + package = self.suite_sourcepackage.sourcepackage > + pocket = self.suite_sourcepackage.pocket > return package.getBranch(pocket) > > @property > def bzr_path(self): > """See `ICanHasLinkedBranch`.""" > - return self._suite_sourcepackage.path > + return self.suite_sourcepackage.path > > def setBranch(self, branch, registrant): > """See `ICanHasLinkedBranch`.""" > - package = self._suite_sourcepackage.sourcepackage > - pocket = self._suite_sourcepackage.pocket > + package = self.suite_sourcepackage.sourcepackage > + pocket = self.suite_sourcepackage.pocket > package.setBranch(pocket, branch, registrant) > > > -class DistributionPackageLinkedBranch: > +class DistributionPackageLinkedBranch(BaseLinkedBranch): > """Implement a linked branch for an `IDistributionSourcePackage`.""" > > adapts(IDistributionSourcePackage) > implements(ICanHasLinkedBranch) > > + sort_order = LinkedBranchOrder.DISTRIBUTION_SOURCE_PACKAGE > + > def __init__(self, distribution_sourcepackage): > - self._distribution_sourcepackage = distribution_sourcepackage > + self.context = distribution_sourcepackage > + > + @property > + def distribution_sourcepackage(self): > + return self.context > + > + def __cmp__(self, other): > + result = super(DistributionPackageLinkedBranch, self).__cmp__(other) > + if result != 0: > + return result > + else: > + my_names = ( > + self.distribution_sourcepackage.distribution.name, > + self.distribution_sourcepackage.sourcepackagename.name) > + other_names = ( > + other.distribution_sourcepackage.distribution.name, > + other.distribution_sourcepackage.sourcepackagename.name) > + return cmp(my_names, other_names) > > @property > def branch(self): > """See `ICanHasLinkedBranch`.""" > development_package = ( > - self._distribution_sourcepackage.development_version) > + self.distribution_sourcepackage.development_version) > if development_package is None: > return None > suite_sourcepackage = development_package.getSuiteSourcePackage( > @@ -123,13 +222,13 @@ > def bzr_path(self): > """See `ICanHasLinkedBranch`.""" > return '/'.join( > - [self._distribution_sourcepackage.distribution.name, > - self._distribution_sourcepackage.sourcepackagename.name]) > + [self.distribution_sourcepackage.distribution.name, > + self.distribution_sourcepackage.sourcepackagename.name]) > > def setBranch(self, branch, registrant): > """See `ICanHasLinkedBranch`.""" > development_package = ( > - self._distribution_sourcepackage.development_version) > + self.distribution_sourcepackage.development_version) > if development_package is None: > raise NoSuchDistroSeries('no current series') > suite_sourcepackage = development_package.getSuiteSourcePackage( > === modified file 'lib/lp/code/model/tests/test_branch.py' > --- lib/lp/code/model/tests/test_branch.py 2010-04-06 17:58:06 +0000 > +++ lib/lp/code/model/tests/test_branch.py 2010-04-08 04:14:00 +0000 > @@ -483,6 +483,203 @@ > self.assertFalse(branch.upgrade_pending) > > > +class TestBranchLinksAndIdentites(TestCaseWithFactory): > + """Test IBranch.branchLinks and IBranch.branchIdentities.""" > + > + layer = DatabaseFunctionalLayer > + > + def test_default_identities(self): > + # If there are no links, the branch identities is just the unique > + # name. This sentence isn't very grammatical, maybe "If there are no links, the only branch identity is the unique name"? > + branch = self.factory.makeAnyBranch() > + self.assertEqual( > + [('lp://dev/' + branch.unique_name, branch)], > + branch.branchIdentities()) > + > + def test_linked_to_product(self): > + # If a branch is linked to the product, it is also by definition > + # linked to the development focus of the product. > + fooix = removeSecurityProxy(self.factory.makeProduct(name='fooix')) > + fooix.development_focus.name = 'devel' > + eric = self.factory.makePerson(name='eric') > + branch = self.factory.makeProductBranch( > + product=fooix, owner=eric, name='trunk') > + linked_branch = ICanHasLinkedBranch(fooix) > + linked_branch.setBranch(branch) > + self.assertEqual( > + [linked_branch, ICanHasLinkedBranch(fooix.development_focus)], > + branch.branchLinks()) > + self.assertEqual( > + [('lp://dev/fooix', fooix), > + ('lp://dev/fooix/devel', fooix.development_focus), > + ('lp://dev/~eric/fooix/trunk', branch)], > + branch.branchIdentities()) > + > + def test_linked_to_product_series(self): > + # If a branch is linked to a non-development series of a product and > + # not linked to the product itself, then the product is not returned > + # in the links. I think the more positive "only the product series is returned in the links" is easier to understand than "the product is not returned". > + fooix = removeSecurityProxy(self.factory.makeProduct(name='fooix')) > + future = self.factory.makeProductSeries(product=fooix, name='future') > + eric = self.factory.makePerson(name='eric') > + branch = self.factory.makeProductBranch( > + product=fooix, owner=eric, name='trunk') > + linked_branch = ICanHasLinkedBranch(future) > + linked_branch.setBranch(branch) > + self.assertEqual( > + [linked_branch], > + branch.branchLinks()) > + self.assertEqual( > + [('lp://dev/fooix/future', future), > + ('lp://dev/~eric/fooix/trunk', branch)], > + branch.branchIdentities()) > + > + def test_linked_to_package(self): > + # If a branch is linked to a suite source package where the > + # distroseries is the current series for the distribution, there is a > + # link for both the distribution source package and the suite source > + # package. > + mint = self.factory.makeDistribution(name='mint') > + dev = self.factory.makeDistroSeries( > + distribution=mint, version='1.0', name='dev') > + eric = self.factory.makePerson(name='eric') > + branch = self.factory.makePackageBranch( > + distroseries=dev, sourcepackagename='choc', name='tip', > + owner=eric) > + dsp = self.factory.makeDistributionSourcePackage('choc', mint) > + distro_link = ICanHasLinkedBranch(dsp) > + development_package = dsp.development_version > + suite_sourcepackage = development_package.getSuiteSourcePackage( > + PackagePublishingPocket.RELEASE) > + suite_sp_link = ICanHasLinkedBranch(suite_sourcepackage) > + > + registrant = getUtility( > + ILaunchpadCelebrities).ubuntu_branches.teamowner > + run_with_login( > + registrant, > + suite_sp_link.setBranch, branch, registrant) > + > + self.assertEqual( > + [distro_link, suite_sp_link], > + branch.branchLinks()) > + self.assertEqual( > + [('lp://dev/mint/choc', dsp), > + ('lp://dev/mint/dev/choc', suite_sourcepackage), > + ('lp://dev/~eric/mint/dev/choc/tip', branch)], > + branch.branchIdentities()) > + > + def test_linked_to_package_not_release_pocket(self): > + # If a branch is linked to a suite source package where the > + # distroseries is the current series for the distribution, but the > + # pocket is not the RELEASE pocket, then there is only the link for > + # the suite source package. > + mint = self.factory.makeDistribution(name='mint') > + dev = self.factory.makeDistroSeries( > + distribution=mint, version='1.0', name='dev') > + eric = self.factory.makePerson(name='eric') > + branch = self.factory.makePackageBranch( > + distroseries=dev, sourcepackagename='choc', name='tip', > + owner=eric) > + dsp = self.factory.makeDistributionSourcePackage('choc', mint) > + development_package = dsp.development_version > + suite_sourcepackage = development_package.getSuiteSourcePackage( > + PackagePublishingPocket.BACKPORTS) > + suite_sp_link = ICanHasLinkedBranch(suite_sourcepackage) > + > + registrant = getUtility( > + ILaunchpadCelebrities).ubuntu_branches.teamowner > + run_with_login( > + registrant, > + suite_sp_link.setBranch, branch, registrant) > + > + self.assertEqual( > + [suite_sp_link], > + branch.branchLinks()) > + self.assertEqual( > + [('lp://dev/mint/dev-backports/choc', suite_sourcepackage), > + ('lp://dev/~eric/mint/dev/choc/tip', branch)], > + branch.branchIdentities()) > + > + def test_linked_to_package_not_current_series(self): > + # If a branch is the development focus branch for a product, then it's > + # bzr identity is lp:product. This comment looks entirely wrong :-) > + mint = self.factory.makeDistribution(name='mint') > + self.factory.makeDistroSeries( > + distribution=mint, version='1.0', name='dev') > + supported = self.factory.makeDistroSeries( > + distribution=mint, version='0.9', name='supported') > + eric = self.factory.makePerson(name='eric') > + branch = self.factory.makePackageBranch( > + distroseries=supported, sourcepackagename='choc', name='tip', > + owner=eric) > + suite_sp = self.factory.makeSuiteSourcePackage( > + distroseries=supported, sourcepackagename='choc', > + pocket=PackagePublishingPocket.RELEASE) > + suite_sp_link = ICanHasLinkedBranch(suite_sp) > + > + registrant = getUtility( > + ILaunchpadCelebrities).ubuntu_branches.teamowner > + run_with_login( > + registrant, > + suite_sp_link.setBranch, branch, registrant) > + > + self.assertEqual( > + [suite_sp_link], > + branch.branchLinks()) > + self.assertEqual( > + [('lp://dev/mint/supported/choc', suite_sp), > + ('lp://dev/~eric/mint/supported/choc/tip', branch)], > + branch.branchIdentities()) > + > + def test_linked_across_project_to_package(self): > + # If a product branch is linked to a suite source package, the links > + # are the same as if it was a source package branch. > + mint = self.factory.makeDistribution(name='mint') > + self.factory.makeDistroSeries( > + distribution=mint, version='1.0', name='dev') > + eric = self.factory.makePerson(name='eric') > + fooix = self.factory.makeProduct(name='fooix') > + branch = self.factory.makeProductBranch( > + product=fooix, owner=eric, name='trunk') > + dsp = self.factory.makeDistributionSourcePackage('choc', mint) > + distro_link = ICanHasLinkedBranch(dsp) > + development_package = dsp.development_version > + suite_sourcepackage = development_package.getSuiteSourcePackage( > + PackagePublishingPocket.RELEASE) > + suite_sp_link = ICanHasLinkedBranch(suite_sourcepackage) > + > + registrant = getUtility( > + ILaunchpadCelebrities).ubuntu_branches.teamowner > + run_with_login( > + registrant, > + suite_sp_link.setBranch, branch, registrant) > + > + self.assertEqual( > + [distro_link, suite_sp_link], > + branch.branchLinks()) > + self.assertEqual( > + [('lp://dev/mint/choc', dsp), > + ('lp://dev/mint/dev/choc', suite_sourcepackage), > + ('lp://dev/~eric/fooix/trunk', branch)], > + branch.branchIdentities()) > + > + def test_junk_branch_links(self): > + # If a junk branch has links, those links are returned in the > + # branchLinks, but the branchIdentities just has the branch unique > + # name. > + eric = self.factory.makePerson(name='eric') > + branch = self.factory.makePersonalBranch(owner=eric, name='foo') > + fooix = removeSecurityProxy(self.factory.makeProduct()) > + linked_branch = ICanHasLinkedBranch(fooix) > + linked_branch.setBranch(branch) > + self.assertEqual( > + [linked_branch, ICanHasLinkedBranch(fooix.development_focus)], > + branch.branchLinks()) > + self.assertEqual( > + [('lp://dev/~eric/+junk/foo', branch)], > + branch.branchIdentities()) > + > + > class TestBzrIdentity(TestCaseWithFactory): > """Test IBranch.bzr_identity.""" > You don't test here any branches that are linked to both products and source packages, say. Is that intentional? I realise the TestLinkedBranchSorting tests basically cover this ground. > === modified file 'lib/lp/code/model/tests/test_linkedbranch.py' > --- lib/lp/code/model/tests/test_linkedbranch.py 2009-08-28 06:39:38 +0000 > +++ lib/lp/code/model/tests/test_linkedbranch.py 2010-04-08 04:14:00 +0000 > @@ -201,5 +201,149 @@ > CannotHaveLinkedBranch, get_linked_branch, project) > > > +class TestLinkedBranchSorting(TestCaseWithFactory): > + > + layer = DatabaseFunctionalLayer > + > + def test_sorting_different_types(self): > + # The different types can be sorted together, and sort so that the > + # results are ordered like: > + # Product Link > + # Distribution Source Package Link > + # Product Series Link > + # Package Link > + product_link = ICanHasLinkedBranch(self.factory.makeProduct()) > + product_series_link = ICanHasLinkedBranch( > + self.factory.makeProductSeries()) > + distro_sp_link = ICanHasLinkedBranch( > + self.factory.makeDistributionSourcePackage()) > + package_link = ICanHasLinkedBranch( > + self.factory.makeSuiteSourcePackage()) > + > + links = sorted( > + [package_link, product_series_link, distro_sp_link, product_link]) > + self.assertIs(product_link, links[0]) > + self.assertIs(distro_sp_link, links[1]) > + self.assertIs(product_series_link, links[2]) > + self.assertIs(package_link, links[3]) > + > + def test_product_sort(self): > + # If in the extremely unlikely event we have one branch linked as the > + # trunk of two different products (you never know), then the sorting Given the test, I think you should say "two or more" here.