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.

• 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 would be good to have a docstring for _matchDDEBs() that tells you that it is a generator method.

• 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.

• 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([]))

With the above points taken into consideration, r=mars