Code review comment for lp:~jml/launchpad/process-upload-checks

Thanks for fixing things! Here's my review:

> + # XXX: JonathanLange 2010-01-15: This shouldn't instantiate policy
> + # types. They should instead have names as class variables.

The XXX should reference a bug :) Or better still, fix it, it's an easy change.

> + # XXX: JonathanLange 2010-01-15: This has to be exactly the same
> + # string as the one in SourcePackageRecipeBuild.policy_name. Factor
> + # out a shared constant.

Same here.

> class SourcePackageRecipeUploadPolicy(BuildDaemonUploadPolicy):

Did you double check the inherited policy options for this? I think it looks sane but I have to ask ...

> + # XXX: JonathanLange 2010-01-15: We should not be re-using magical
> + # string literals. Zombie Dijkstra will come and kill us in our sleep.

bug number *cough* :)

> - buildqueue_record = Attribute("Corespondent BuildQueue record")
> + buildqueue_record = Attribute("Corresponding BuildQueue record")

I know you're just fixing the text, but we can surely do better than "Attribute".

> + policy_name = Attribute(
> + "The upload policy to use for handling these builds.")

And here!

> + uploader_argv = list(config.builddmaster.uploader.split())

Can you rename this variable to something like "uploader_command". The "argv" bit made me look twice since I was expecting only parameters to the command but it's actually the base command from the config.

> + # XXX: Duplicated from getUploaderCommand
> uploader_logfilename = os.path.join(upload_dir, 'uploader.log')

Why not turn it into a property on the class then?

> - classProvides(IBuildFarmCandidateJobSelection, ISpecificBuildFarmJobClass)
> + classProvides(ISpecificBuildFarmJobClass)

Why did you remove that? I note that it's there in db-devel but not devel.

> + # XXX: BuildBase is supposed to implement IBuildBase, but doesn't atm.
> + # Since it's not the focus of the branch, we'll postpone the work.

From the code I just looked at, it does! If there's a genuine problem then we need a bug please - tag wellington of course. This is becoming known as the Wellington project I think!

FWIW, I think all classes should have basic doctests, I really don't like this unit testing for their basic features.

What layer is TestBuildBase going to run in if it's not explicitly specified?

> +class TestBuildBaseHarder(TestCaseWithFactory):

Crappy name alert! Perhaps "TestBuildBaseWithDatabase" ?

Or maybe, s/TestBuildBase/TestBuildBase<layer>/ where <layer> is whatever it's really using (and I think it should be explicit) and then s/TestBuildBaseHarder/TestBuildBase/. Your call.

> + <adapter factory="lp.soyuz.model.recipebuilder.RecipeBuildBehavior"
> + permission="zope.Public" />

You can fill a gap in my knowledge here, what does this do when declared without the "provides" and "for" ?

> + build_log_url = exported(

Why is this moved from BuildBase to Build?

> + # JRV 2010-01-15: The database table really should have a pocket
> + # column, although this is not a big problem at the moment as recipe
> + # builds only happen for PPA's (so far). (bug 507307)

Can you XXX this in the usual format please.

I guess I should tell Jelmer how this works too :)

> + distroseries.newArch(
> + '386', processorfamily, True, self.factory.makePerson())

Nitpick, but s/386/i386/ for the architecture tag.

Looking good! Can you answer my queries above please and then we'll get this puppy landed.

Cheers
J