Personal thoughts:
On Dec 14, 2009, at 7:22 PM, Chris Jerdonek wrote:
I have a few questions related to patch length: (1) Do reviewers take patch length into account when considering whether to review a patch? This is useful to know for those who would rather have a short patch reviewed more quickly than wait longer for a longer patch to be reviewed.
Yes.
(2) If reviewers do take patch length into account, what's the best way to handle trivial changes that might inflate patch length (for example moving a large chunk of code or adding an image) -- should such changes be submitted separately?
If they are meaningful to do separately (i.e. tree won't be in a ridiculous or broken state) then sure. In particular copying or renaming files or doing large code cleanup is best done separate from substantive changes.
It can also help to mention if parts of the patch are mechanical and identify just the most meaningful parts.
Another thing that makes it easier for me review is test cases. If I can see exactly what behavior change is intended in the form of a test case, it's easier to evaluate the code changes. This is true even if adding numerous test cases makes the code change overall bigger.
(3) Finally, in people's experience, what is the "sweet spot" for patch length? (There is an optimization problem here somewhere.)
I make some effort to break a patch down into the smallest meaningful pieces I can if it seems large, even if I have to do this after the fact. I particularly like to separate preparatory refactorings from actual behavior changes. On the other hand, if some changes really are tied to each other and aren't meaningful separately, I usually bite the bullet.
Regards, Maciej _______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
