Code review comment for lp:~jelmer/launchpad/613468-xb-ppa-db

Hi Jelmer! Nice clean branch :)

> === modified file 'lib/lp/soyuz/interfaces/binarypackagebuild.py'
> --- lib/lp/soyuz/interfaces/binarypackagebuild.py 2010-08-10 21:54:41 +0000
> +++ lib/lp/soyuz/interfaces/binarypackagebuild.py 2010-08-12 10:37:50 +0000
> @@ -125,10 +125,11 @@
>
> def createBinaryPackageRelease(
> binarypackagename, version, summary, description, binpackageformat,
> - component, section, priority, shlibdeps, depends, recommends,
> - suggests, conflicts, replaces, provides, pre_depends, enhances,
> - breaks, essential, installedsize, architecturespecific,
> - debug_package):
> + component, section, priority, installedsize, architecturespecific,
> + shlibdeps=None, depends=None, recommends=None, suggests=None,
> + conflicts=None, replaces=None, provides=None, pre_depends=None,
> + enhances=None, breaks=None, essential=False, debug_package=None,
> + user_defined_fields=None):

I'm assuming you've checked that all call-sites use kwargs before re-ordering... and my next question was going to be are you sure that all those kwargs are really optional, but your test_provides() ensures that too :)

> """Create and return a `BinaryPackageRelease`.
>
> The binarypackagerelease will be attached to this specific build.

>
> === modified file 'lib/lp/soyuz/interfaces/binarypackagerelease.py'
> --- lib/lp/soyuz/interfaces/binarypackagerelease.py 2010-07-10 04:52:43 +0000
> +++ lib/lp/soyuz/interfaces/binarypackagerelease.py 2010-08-12 10:37:50 +0000
> @@ -56,6 +56,8 @@
> title=_("Debug package"), schema=Interface, required=False,
> description=_("The corresponding package containing debug symbols "
> "for this binary."))
> + user_defined_fields = Attribute(
> + "Sequence of user-defined fields as key-value pairs.")

you could use zope.schema.List instead of attribute:

    user_defined_fields = List(
        title=_("Sequence of user-defined fields as key-value pairs."))

if that's what it is.

>
> files = Attribute("Related list of IBinaryPackageFile entries")
>
>
> === modified file 'lib/lp/soyuz/interfaces/sourcepackagerelease.py'
> --- lib/lp/soyuz/interfaces/sourcepackagerelease.py 2010-07-29 22:55:15 +0000
> +++ lib/lp/soyuz/interfaces/sourcepackagerelease.py 2010-08-12 10:37:50 +0000
> @@ -96,8 +96,8 @@
> "was first uploaded in Launchpad")
> publishings = Attribute("MultipleJoin on SourcepackagePublishing")
>
> -
> -
> + user_defined_fields = Attribute(
> + "Sequence of user-defined fields as key-value pairs.")

Same here.

> # read-only properties
> name = Attribute('The sourcepackagename for this release, as text')
> title = Attribute('The title of this sourcepackagerelease')
>
> === modified file 'lib/lp/soyuz/model/binarypackagerelease.py'
> --- lib/lp/soyuz/model/binarypackagerelease.py 2010-07-10 04:46:49 +0000
> +++ lib/lp/soyuz/model/binarypackagerelease.py 2010-08-12 10:37:50 +0000
> @@ -13,6 +13,8 @@
>
> from zope.interface import implements
>
> +import simplejson
> +
> from sqlobject import StringCol, ForeignKey, IntCol, SQLMultipleJoin, BoolCol
> from storm.locals import Date, Int, Reference, Storm
>
> @@ -68,6 +70,22 @@
> files = SQLMultipleJoin('BinaryPackageFile',
> joinColumn='binarypackagerelease', orderBy="libraryfile")
>
> + _user_defined_fields = StringCol(dbName='user_defined_fields')
> +
> + def __init__(self, *args, **kwargs):
> + if 'user_defined_fields' in kwargs:
> + kwargs['_user_defined_fields'] = simplejson.dumps(
> + kwargs['user_defined_fields'])
> + del kwargs['user_defined_fields']
> + SQLBase.__init__(self, *args, **kwargs)

Do you prefer explicitly calling the superclass rather than using super? I don't mind if you do, but otherwise I'd go for super (https://dev.launchpad.net/PythonStyleGuide#Chaining%20method%20calls)

> === modified file 'lib/lp/soyuz/model/sourcepackagerelease.py'
> --- lib/lp/soyuz/model/sourcepackagerelease.py 2010-08-02 02:13:52 +0000
> +++ lib/lp/soyuz/model/sourcepackagerelease.py 2010-08-12 10:37:50 +0000
> @@ -15,6 +15,7 @@
> import pytz
> from StringIO import StringIO
> import re
> +import simplejson
>
> from sqlobject import StringCol, ForeignKey, SQLMultipleJoin
> from storm.expr import Join
> @@ -147,6 +148,22 @@
> package_diffs = SQLMultipleJoin(
> 'PackageDiff', joinColumn='to_source', orderBy="-date_requested")
>
> + _user_defined_fields = StringCol(dbName='user_defined_fields')
> +
> + def __init__(self, *args, **kwargs):
> + if 'user_defined_fields' in kwargs:
> + kwargs['_user_defined_fields'] = simplejson.dumps(
> + kwargs['user_defined_fields'])
> + del kwargs['user_defined_fields']
> + SQLBase.__init__(self, *args, **kwargs)

And again.

> === added file 'lib/lp/soyuz/tests/test_binarypackagerelease.py'

Yay... nice unit tests.

Thanks Jelmer!