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

 review approve
 merge approve

> As a meta-comment, I'd really appreciate it if you could leave the
> filenames from the diff in your reply. It makes it much, much faster
> to find the relevant code in my editor.

Right, sorry, I hate that too.

> Bug filed. It's an easy change, but the branch is big enough as-is.
> I'll make the change in a separate branch.

Cool, thank you.

> self.unsigned_changes_ok = True
> self.unsigned_dsc_ok = True
> self.can_upload_source = False
> self.can_upload_mixed = False
>
> I don't know if those bits are good. Your call.

That looks fine, thanks for checking (again).

> >> - 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".
>
> Changed to Object(...)

Isn't it a Reference() ?

> Ahh ok. FWIW, I used "argv" specifically because it corresponds to
> argv arrays in C convention by including the argument.
>
> Changed to uploader_command.

True. I always hated that :)

>
> >> + # XXX: Duplicated from getUploaderCommand
> >> uploader_logfilename = os.path.join(upload_dir, 'uploader.log')
> >
> > Why not turn it into a property on the class then?
>
> Because it's parametrized -- it depends on upload_leaf. I've made it
> an argument to getUploaderCommand.

D'oh, right.

> >> - classProvides(IBuildFarmCandidateJobSelection,
> >> ISpecificBuildFarmJobClass) + classProvides(ISpecificBuildFarmJobClass)
> >
> > Why did you remove that? I note that it's there in db-devel but not
> > devel.
>
> _Probably_ a bad merge. Restored.

Phew, thanks ;)

Since it didn't break anything, this indicates a missing test :/

> It didn't when I submitted this branch last week, and when I
> re-enabled the test, it failed. The problem is genuine, so I've filed
> a bug and added it to the comment.

Argh, ok, thanks. I was just looking at the "implements" rather than actually
looking to see what it was implementing!

> > FWIW, I think all classes should have basic doctests, I really don't like
> > this unit testing for their basic features.
>
> OK. I haven't added any doctests in this branch.

Yeah, it's a matter of policy I'll raise in a reviewers' meeting I think.

> > What layer is TestBuildBase going to run in if it's not explicitly
> > specified?
>
> No layer at all. It doesn't need to be run in a layer. We want more
> tests like this, because they are fast :)

Ah! Ok, that's cool. We need more of this! You should totally email the
list about that.

> I forwarded you an email on the subject from the Canonical thread.
> http://www.muthukadan.net/docs/zca.html#zcml has some other notes.

Right, I remember it now, thanks for the pointer.

> >> + build_log_url = exported(
> >
> > Why is this moved from BuildBase to Build?
>
> When I was trying to get the assertProvides(BuildBase, IBuildBase)
> thing to pass, it turned out that BuildBase didn't implement this, so
> I moved the declaration.

Righto.

> I look forward to hearing back from you.

Land it!

Cheers,
J.