Code review comment for lp:~wgrant/launchpad/test-ddeb-matching

> Hi William,
>
> This is a meaty change, lots of new code. Very nice. I especially like the
> refactoring of NascentUpload and _matchDDEBs(): both excellent changes.
>
> Here is the list of notes I took while reviewing the code:
>
> • Do ChangesFile objects have a .path attribute that you can use instead of
> string-sniffing in the constructor? If so, then perhaps you can push the
> fiddly conditional construction down into ChangesFile itself and at the same
> time eliminate the .changesfile_path attribute from the NascentUpload object.
> A NascentUpload.from_path() factory method would be even better, eliminating
> the conditional construction entirely.

Both fixed. All path-based NascentUpload() callsites updated to use NascentUpload.from_changesfile_path() instead.

> • Switching from .reject() to 'yield UploadError()' is a pretty big API
> change, however I do not see any code in the diff (besides the tests) that is
> impacted by it. Why not?

It's a pattern used throughout archiveuploader -- we call
run_and_collect_errors on an error generator, and it rejects with all the
yielded errors.

> • It would be good to have a docstring for _matchDDEBs() that tells you that
> it is a generator method.

Missed that. Fixed.

> • I think the test names could be improved a bit: the comment should be used
> as test method name, because the comments tell you what the desired behaviour
> is. They tell you what the test is actually testing.

Fair point. Fixed.

> • You might be able to rewrite all those "self.assertEquals([],
> list(self.upload._matchDDEBs()))" statements using the nifty
> testtools.TestCase.assertThat() method: self.assertThat(self.upload,
> matchesDDEBs([]))

I opted to instead create a tiny domain-specific assertion method.

> With the above points taken into consideration, r=mars

Thanks.