Code review comment for lp:~adiroiban/launchpad/bug-531261

Hi Adi,

This is a nice refactoring; thanks for doing it.

There's just one thing I noticed which was not mentioned in your cover
letter: the IProductSeries interface has grown a 'active' attribute,
thanks to ISeriesMixin. Since your branch doesn't add any code that
uses that attribute, I don't see a reason for adding it, so I'd suggest
moving it to IDistroSeries and the actual implementation to
DistroSeries. Unless you do plan to use IProductSeries.active
somewhere?

On Fri, 2010-03-05 at 13:51 +0000, Adi Roiban wrote:
> Adi Roiban has proposed merging lp:~adiroiban/launchpad/bug-531261
> into lp:launchpad/devel.
>
[...]
> == Implementation details ==
>
> I don't know how to fix pylint warning about lazr modules. Any hint is
> much appreciated.

These are spurious, but I don't know how to silence pylint,
unfortunately.

>
> == Tests ==
> ./bin/test -t series
>
> == Demo and Q/A ==
>
> Since this is only a cleanup, there is no new functionality to test.
>
> = Launchpad lint =
>
> Checking for conflicts. and issues in doctests and templates.
> Running jslint, xmllint, pyflakes, and pylint.
> Using normal rules.
>
> Linting changed files:
> lib/lp/registry/interfaces/distroseries.py
>
>
> == Pylint notices ==
>
> lib/lp/registry/interfaces/distroseries.py
> 22: [F0401] Unable to import 'lazr.enum' (No module named enum)
> 23: [F0401] Unable to import 'lazr.restful.declarations' (No
> module named restful)
> 28: [F0401] Unable to import 'lazr.restful.fields' (No module
> named restful)
> 112: [E1002, DistroSeriesVersionField._validate] Use super on an
> old style class

> === modified file 'lib/lp/registry/interfaces/series.py'
> --- lib/lp/registry/interfaces/series.py 2009-12-13 11:55:40 +0000
> +++ lib/lp/registry/interfaces/series.py 2010-03-05 13:51:28 +0000
> @@ -76,3 +88,45 @@
> haven't started working yet.
> """)
>
> +
> +class ISeriesMixin(IHasDrivers):
> + """Methods & properties shared between distro & product series."""
> +
> + active = exported(
> + Bool(

In this case, having the first argument on a line by itself doesn't buy
us much as it allows you to move only one char to the left, so better to
leave it on the same line as Bool.

> + title=_("Active"),
> + description=_(
> + "Whether or not this series is stable and supported, or "
> + "under current development. This excludes series which "
> + "are experimental or obsolete.")))
> +
> + summary = exported(
> + Summary(title=_("Summary"),
> + description=_('A single paragraph that explains the goals of '
> + 'of this series and the intended users. '
> + 'For example: "The 2.0 series of Apache '
> + 'represents the current stable series, '
> + 'and is recommended for all new deployments".'),
> + required=True))
> +
> + drivers = exported(
> + CollectionField(
> + title=_(
> + 'A list of the people or teams who are drivers for this '
> + 'series. This list is made up of any drivers or owners '
> + 'from this series and the parent drivers.'),
> + readonly=True,
> + value_type=Reference(schema=IPerson)))
> +
> +
> + bug_supervisor = CollectionField(
> + title=_('Currently just a reference to the parent bug '
> + 'supervisor.'),
> + readonly=True,
> + value_type=Reference(schema=IPerson))
> +
> + security_contact = PublicPersonChoice(
> + title=_('Security Contact'),
> + description=_('Currently just a reference to the parent '
> + 'security contact.'),
> + required=False, vocabulary='ValidPersonOrTeam')
>

--
Guilherme Salgado <email address hidden>