launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Approve code



Diff comments:

> 
> === modified file 'lib/lp/snappy/model/snappyseries.py'
> --- lib/lp/snappy/model/snappyseries.py	2016-05-24 05:15:50 +0000
> +++ lib/lp/snappy/model/snappyseries.py	2016-11-08 16:52:28 +0000
> @@ -61,25 +62,55 @@
>      status = EnumCol(enum=SeriesStatus, notNull=True)
>  
>      def __init__(self, registrant, name, display_name, status,
> -                 date_created=DEFAULT):
> +                 preferred_distro_series=None, date_created=DEFAULT):
>          super(SnappySeries, self).__init__()
>          self.registrant = registrant
>          self.name = name
>          self.display_name = display_name
>          self.status = status
>          self.date_created = date_created
> +        self.preferred_distro_series = preferred_distro_series
>  
>      @property
>      def title(self):
>          return self.display_name
>  
>      @property
> +    def preferred_distro_series(self):
> +        row = Store.of(self).find(
> +            SnappyDistroSeries,

Can you just use (SnappyDistroSeries.distro_series,) here and ditch the last line of the method?

> +            SnappyDistroSeries.snappy_series == self,
> +            SnappyDistroSeries.preferred == True).one()
> +        return row.distro_series if row is not None else None
> +
> +    @preferred_distro_series.setter
> +    def preferred_distro_series(self, value):
> +        current = Store.of(self).find(
> +            SnappyDistroSeries,
> +            SnappyDistroSeries.snappy_series == self,
> +            SnappyDistroSeries.preferred == True).one()
> +        if current is not None:
> +            if current.distro_series == value:
> +                return
> +            current.preferred = False
> +        if value is not None:
> +            row = Store.of(self).find(
> +                SnappyDistroSeries,
> +                SnappyDistroSeries.snappy_series == self,
> +                SnappyDistroSeries.distro_series == value).one()
> +            if row is not None:
> +                row.preferred = True
> +            else:
> +                row = SnappyDistroSeries(self, value, preferred=True)
> +                Store.of(self).add(row)

The automatic add behaviour seems slightly weird, but I suppose it's fine.

Do we need to cachedproperty any of this?

> +
> +    @property
>      def usable_distro_series(self):
>          rows = IStore(DistroSeries).find(
>              DistroSeries,
>              SnappyDistroSeries.snappy_series == self,
>              SnappyDistroSeries.distro_series_id == DistroSeries.id)
> -        return rows.order_by(DistroSeries.id)
> +        return rows.order_by(Desc(DistroSeries.id))
>  
>      @usable_distro_series.setter
>      def usable_distro_series(self, value):


-- 
https://code.launchpad.net/~cjwatson/launchpad/snap-preferred-series/+merge/310216
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.