launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Approve code



Diff comments:

> 
> === modified file 'lib/lp/registry/model/distroseries.py'
> --- lib/lp/registry/model/distroseries.py	2015-10-13 13:22:08 +0000
> +++ lib/lp/registry/model/distroseries.py	2015-11-21 11:14:12 +0000
> @@ -790,6 +794,38 @@
>              distroseries=self, type=LanguagePackType.DELTA,
>              updates=self.language_pack_base, orderBy='-date_exported')
>  
> +    @property
> +    def backports_not_automatic(self):
> +        if self.publishing_options is not None:
> +            return self.publishing_options.get(
> +                "backports_not_automatic", False)
> +        else:
> +            return self._backports_not_automatic
> +
> +    @backports_not_automatic.setter
> +    def backports_not_automatic(self, value):
> +        assert isinstance(value, bool)
> +        if self.publishing_options is not None:
> +            self.publishing_options["backports_not_automatic"] = value
> +        else:
> +            self._backports_not_automatic = value
> +
> +    @property
> +    def include_long_descriptions(self):
> +        if self.publishing_options is not None:
> +            return self.publishing_options.get(
> +                "include_long_descriptions", True)
> +        else:
> +            return self._include_long_descriptions
> +
> +    @include_long_descriptions.setter
> +    def include_long_descriptions(self, value):
> +        assert isinstance(value, bool)
> +        if self.publishing_options is not None:
> +            self.publishing_options["include_long_descriptions"] = value
> +        else:
> +            self._include_long_descriptions = value

This is buggy on new distroseries, as publishing_options is never set to the empty dict.

It also seems like the sort of thing that could use a custom descriptor once the transition code is removed. We could use it in a few places.

> +
>      def _customizeSearchParams(self, search_params):
>          """Customize `search_params` for this distribution series."""
>          search_params.setDistroSeries(self)
> 
> === modified file 'lib/lp/scripts/garbo.py'
> --- lib/lp/scripts/garbo.py	2015-10-01 07:17:36 +0000
> +++ lib/lp/scripts/garbo.py	2015-11-21 11:14:12 +0000
> @@ -1461,6 +1462,40 @@
>          transaction.commit()
>  
>  
> +class DistroSeriesPublishingOptionsPopulator(TunableLoop):
> +    """Populates DistroSeries.publishing_options."""
> +
> +    maximum_chunk_size = 5000
> +
> +    def __init__(self, log, abort_time=None):
> +        super(DistroSeriesPublishingOptionsPopulator, self).__init__(
> +            log, abort_time)
> +        self.start_at = 1
> +        self.store = IMasterStore(DistroSeries)
> +
> +    def findSeries(self):
> +        return self.store.find(
> +            DistroSeries,
> +            DistroSeries.id >= self.start_at).order_by(DistroSeries.id)
> +
> +    def isDone(self):
> +        return self.findSeries().is_empty()
> +
> +    def __call__(self, chunk_size):
> +        all_series = list(self.findSeries()[:chunk_size])
> +        for series in all_series:
> +            if series.publishing_options is None:
> +                naked_series = removeSecurityProxy(series)
> +                series.publishing_options = {
> +                    "backports_not_automatic":
> +                        naked_series._backports_not_automatic,
> +                    "include_long_descriptions":
> +                        naked_series._include_long_descriptions,
> +                    }
> +        self.start_at = all_series[-1].id + 1
> +        transaction.commit()

A garbo job is sorta overkill, but it works.

It's slightly weird in that creating a fresh distroseries (not using IDS) will actually leave the JSON empty, while this will set both values on all existing series, but the accessors handle both.

> +
> +
>  class BaseDatabaseGarbageCollector(LaunchpadCronScript):
>      """Abstract base class to run a collection of TunableLoops."""
>      script_name = None  # Script name for locking and database user. Override.


-- 
https://code.launchpad.net/~cjwatson/launchpad/distroseries-publishing-options/+merge/278242
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.