launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Approve code



Diff comments:

> 
> === modified file 'lib/lp/registry/vocabularies.py'
> --- lib/lp/registry/vocabularies.py	2016-07-27 17:19:20 +0000
> +++ lib/lp/registry/vocabularies.py	2016-09-07 22:41:38 +0000
> @@ -2089,18 +2089,38 @@
>              dsp = spn_or_dsp
>          elif spn_or_dsp is not None:
>              dsp = self.distribution.getSourcePackage(spn_or_dsp)
> -        if dsp is not None and (dsp == self.dsp or dsp.is_official):
> -            if binary_names:
> -                # Search already did the hard work of looking up binary names.
> -                cache = get_property_cache(dsp)
> -                cache.binary_names = binary_names
> -            # XXX cjwatson 2016-07-22: It's a bit odd for the token to
> -            # return just the source package name and not the distribution
> -            # name as well, but at the moment this is always fed into a
> -            # package name box so things work much better this way.  If we
> -            # ever do a true combined distribution/package picker, then this
> -            # may need to be revisited.
> -            return SimpleTerm(dsp, dsp.name, dsp.name)
> +        if dsp is not None:
> +            if dsp == self.dsp or dsp.is_official:
> +                if binary_names:
> +                    # Search already did the hard work of looking up binary
> +                    # names.
> +                    cache = get_property_cache(dsp)
> +                    cache.binary_names = binary_names
> +                # XXX cjwatson 2016-07-22: It's a bit odd for the token to
> +                # return just the source package name and not the
> +                # distribution name as well, but at the moment this is
> +                # always fed into a package name box so things work much
> +                # better this way.  If we ever do a true combined
> +                # distribution/package picker, then this may need to be
> +                # revisited.
> +                return SimpleTerm(dsp, dsp.name, dsp.name)
> +            else:
> +                # Does this vocabulary have any package names at all?
> +                empty = IStore(DistributionSourcePackageCache).find(
> +                    Or(
> +                        DistributionSourcePackageCache.archiveID.is_in(
> +                            self.distribution.all_distro_archive_ids),
> +                        DistributionSourcePackageCache.archive == None),
> +                    DistributionSourcePackageCache.distribution ==
> +                        self.distribution).is_empty()

This location constraint feels like it could be factored out of searchForTerms, but maybe that's no less ugly.

> +                if empty:
> +                    # If the vocabulary has no package names, then this is
> +                    # probably a distribution not managed in Launchpad.  In
> +                    # that case we are more liberal about allowing unknown
> +                    # package names, in order to support existing uses such
> +                    # as noting that the same bug exists in the same package
> +                    # in multiple distributions.
> +                    return SimpleTerm(dsp, dsp.name, dsp.name)
>          raise LookupError(self.distribution, spn_or_dsp)
>  
>      def getTerm(self, spn_or_dsp):


-- 
https://code.launchpad.net/~cjwatson/launchpad/distribution-filebug-dsp-vocab/+merge/305150
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.