Code review comment for lp:~abentley/launchpad/incremental-diffs

Revision history for this message
Leonard Richardson (leonardr) wrote :

This branch looks good with some trivial cleanup suggested in IRC, one minor change also suggested in IRC, and one major caveat:

The minor change: test_generateIncrementalDiff is pretty confusing, with lots of branches and lots of nearly-identical commits. It needs at least a comment up-front describing what's in the two revisions being diffed, and how the precursor and prerequisite branches affect this.

It might also be easier to understand if you defined a helper method to create a new branch *and its first commit*. This would make it clear which commits are setup, reflecting the "initial" states of the branches, and which commits are the ones leading up to the endpoint of the incremental diff. But an explanatory comment is good enough to get my approval.

The caveat: this branch introduces a new source dependency, difftacular, which was written by Aaron in July. Since it was written in-house, I think difftacular needs to go through the code review process, just like the lazr packages that are used in Launchpad. Fortunately, the entire difftacular project isn't much longer than 800 lines, so it shouldn't be too difficult for me to do the review.

review: Approve

« Back to merge proposal