Code review comment for lp:~wgrant/launchpad/gina-3.0-support

 review approve
 merge approve

> gina needs to support 3.0 (quilt) and 3.0 (native) sources. To do this, it
> just needs to identify files as the correct types. Since archiveuploader
> already has a function to do this, I've altered gina to use that rather
> than the duplicated canonical.launchpad.helpers.getFileType. As gina was
> the final callsite of getFileType, I've removed that.
>
> Since determine_source_file_type only does source files, I've added
> determine_binary_file_type. It could possibly go along with
> determine_source_file_type in lp.archiveuploader.utils, but I'm not quite
> sure.

I think it should go there to be consistent.

>
> For testing, I've added a 3.0 (quilt) source to the test archive.
>

Great, thanks for making this change William! It looks good to me so I
approved it. There's just a couple of minor comments inline.

[snip]

>=== modified file 'lib/lp/soyuz/scripts/gina/handlers.py'
>--- lib/lp/soyuz/scripts/gina/handlers.py 2009-11-07 09:50:37 +0000
>+++ lib/lp/soyuz/scripts/gina/handlers.py 2009-12-01 10:10:48 +0000
>@@ -29,6 +29,7 @@
>
> from lp.archivepublisher.diskpool import poolify
> from lp.archiveuploader.tagfiles import parse_tagfile
>+from lp.archiveuploader.utils import determine_source_file_type
>
> from canonical.database.sqlbase import sqlvalues
>
>@@ -46,10 +47,22 @@
>
> from lp.registry.interfaces.person import IPersonSet,
> PersonCreationRationale from lp.registry.interfaces.sourcepackage import
> SourcePackageType +from lp.soyuz.interfaces.binarypackagerelease import
> BinaryPackageFileType from lp.soyuz.interfaces.binarypackagename import
> IBinaryPackageNameSet from lp.soyuz.interfaces.build import BuildStatus
> from lp.soyuz.interfaces.publishing import PackagePublishingStatus
>-from canonical.launchpad.helpers import getFileType, getBinaryPackageFormat
>+from canonical.launchpad.helpers import getBinaryPackageFormat

I think Gina is the only thing using getBinaryPackageFormat - it might be
worth de-cluttering helpers.py and moving it to the gina module.

>+
>+
>+def determine_binary_file_type(filename):
>+ """Determine the BinaryPackageFileType of the given filename."""
>+
>+ if filename.endswith(".deb"):
>+ return BinaryPackageFileType.DEB
>+ elif filename.endswith(".udeb"):
>+ return BinaryPackageFileType.DEB

This should be UDEB I think?

Everything else looks good! Remind me to land this and the other branch when
PQM opens.

Cheers
J