Code review comment for lp:~wgrant/launchpad/distroseries-source-format-selection-part1

> Hi William, thanks for making this change. There's some much-needed
> refactoring and clearing up in nascentupload.
>
> The basic direction is good, I just have a few comments to make for things
> that need fixing. Gavin will do a more thorough review as we agreed on IRC.
>
> General questions:
>
> * I can't tell from the diff but are the db permissions added for the new
> table correctly tested in the tests? This usually just means ensuring that
> the test runs as the correct db user.

I'll have to look at that later. I'm not quite sure.

> * The XXX about the orphaned files is interesting. I don't think we deal
> with this right now do we? I'm trying to think if it would be bad to reject
> the upload if we detect orphan files, and I can't think of one. What's your
> opinion?

I think there is a word or two missing there. You mean you can't think of a reason that it would be bad, or that it wouldn't be bad?

I can't see any compelling reason to reject if it has extra files, apart from it seeming slightly cleaner. If you feel it needs to be done, it's probably easy enough to add.

> Things that need fixing:
>
> * The upload format check is not tested. (The "%s: format '%s' is not
> permitted in %s." one)

Ah, yes, forgot about that. The other half of the branch has 3.0 format test source packages, and tests that. I suppose I should bring one of those in.

> * IDistroSeries methods permitSourcePackageFormat() and
> isSourcePackageFormatPermitted() should be in a utility, e.g.
> SourcePackageFormatSet. This ensures that in a zopeful environment they are
> security wrapped. Although the upload processor is zopeless, we might need to
> do that one day. Something like
> * SourcePackageFormatSet.add()
> * SourcePackageFormatSet.getByFormat()
> would fit with our current style. You can keep the
> isSourcePackageFormatPermitted() and make it call the utility method as a
> convenience, but the permitSourcePackageFormat() should go.
>
> * Once that's done, you should remove the __init__ on the SourcePackageFormat
> model class and make the new utility's add() method initialise the correct
> fields.

OK, sure. Will fix.

> Everything else looks great, thanks!

Thanks for the review.