launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Review: Approve



Diff comments:

> diff --git a/lib/lp/bugs/model/bugnomination.py b/lib/lp/bugs/model/bugnomination.py
> index 4c322cd..3cd7868 100644
> --- a/lib/lp/bugs/model/bugnomination.py
> +++ b/lib/lp/bugs/model/bugnomination.py
> @@ -32,37 +33,62 @@ from lp.bugs.interfaces.bugnomination import (
>      IBugNominationSet,
>      )
>  from lp.bugs.interfaces.bugtask import IBugTaskSet
> +from lp.registry.interfaces.distroseries import IDistroSeries
>  from lp.registry.interfaces.person import validate_public_person
> +from lp.registry.interfaces.productseries import IProductSeries
> +from lp.registry.interfaces.sourcepackage import ISourcePackage
>  from lp.services.database.constants import UTC_NOW
> -from lp.services.database.datetimecol import UtcDateTimeCol
> -from lp.services.database.enumcol import EnumCol
> -from lp.services.database.sqlbase import SQLBase
> +from lp.services.database.enumcol import DBEnum
> +from lp.services.database.interfaces import IStore
> +from lp.services.database.stormbase import StormBase
>  from lp.services.features import getFeatureFlag
>  
>  
>  @implementer(IBugNomination)
> -class BugNomination(SQLBase):
> -    _table = "BugNomination"
> -
> -    owner = ForeignKey(
> -        dbName='owner', foreignKey='Person',
> -        storm_validator=validate_public_person, notNull=True)
> -    decider = ForeignKey(
> -        dbName='decider', foreignKey='Person',
> -        storm_validator=validate_public_person, notNull=False, default=None)
> -    date_created = UtcDateTimeCol(notNull=True, default=UTC_NOW)
> -    date_decided = UtcDateTimeCol(notNull=False, default=None)
> -    distroseries = ForeignKey(
> -        dbName='distroseries', foreignKey='DistroSeries',
> -        notNull=False, default=None)
> -    productseries = ForeignKey(
> -        dbName='productseries', foreignKey='ProductSeries',
> -        notNull=False, default=None)
> -    bug = ForeignKey(dbName='bug', foreignKey='Bug', notNull=True)
> -    status = EnumCol(
> -        dbName='status', notNull=True, schema=BugNominationStatus,
> +class BugNomination(StormBase):
> +
> +    __storm_table__ = "BugNomination"
> +
> +    id = Int(primary=True)
> +
> +    owner_id = Int(
> +        name="owner", allow_none=False, validator=validate_public_person)
> +    owner = Reference(owner_id, "Person.id")
> +
> +    decider_id = Int(
> +        name="decider", allow_none=True, default=None,
> +        validator=validate_public_person)
> +    decider = Reference(decider_id, "Person.id")
> +
> +    date_created = DateTime(allow_none=False, default=UTC_NOW, tzinfo=pytz.UTC)
> +    date_decided = DateTime(allow_none=True, default=None, tzinfo=pytz.UTC)
> +
> +    distroseries_id = Int(name="distroseries", allow_none=True, default=None)
> +    distroseries = Reference(distroseries_id, "DistroSeries.id")
> +
> +    productseries_id = Int(name="productseries", allow_none=True, default=None)
> +    productseries = Reference(productseries_id, "ProductSeries.id")
> +
> +    bug_id = Int(name='bug', allow_none=False)
> +    bug = Reference(bug_id, 'Bug.id')
> +
> +    status = DBEnum(
> +        name='status', allow_none=False, enum=BugNominationStatus,
>          default=BugNominationStatus.PROPOSED)
>  
> +    def __init__(self, owner=None, decider=None, date_created=UTC_NOW,
> +                 date_decided=None, distroseries=None,
> +                 productseries=None, bug=None,
> +                 status=BugNominationStatus.PROPOSED):

I normally prefer declaring the arguments you have to supply (because they're `allow_none=False` and have no default) as positional arguments rather than keyword arguments.  In this case that would be `owner` and `bug`.

> +        self.owner = owner
> +        self.decider = decider
> +        self.date_created = date_created
> +        self.date_decided = date_decided
> +        self.distroseries = distroseries
> +        self.productseries = productseries
> +        self.bug = bug
> +        self.status = status
> +
>      @property
>      def target(self):
>          """See IBugNomination."""
> @@ -156,6 +182,10 @@ class BugNomination(SQLBase):
>                      return True
>          return False
>  
> +    def __repr__(self):
> +        return "<BugNomination at 0x%x bug=%s owner=%s>" % (
> +            id(self), self.bug_id, self.owner_id)

The "at 0x%x" bit is in `object`'s default __repr__, but we don't need to and arguably shouldn't emulate it here, even if removing it does require fixing up some doctests.  (See https://bugs.launchpad.net/launchpad/+bug/1635787.)

> +
>  
>  @implementer(IBugNominationSet)
>  class BugNominationSet:


-- 
https://code.launchpad.net/~pappacena/launchpad/+git/launchpad/+merge/395021
Your team Launchpad code reviewers is subscribed to branch ~pappacena/launchpad:stormify-bugnomination.