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
