Re: Lessons from commit fest

Gregory Stark <stark(at)enterprisedb(dot)com> writes:
> I don't think we know what a "typical review" really looks like.

A general comment is that in stuff I review, I frequently spend a lot of
time trying to make the patch "look like it belongs", that is make it
reasonably well-integrated with the surrounding code. This is important
because a code base that too obviously consists of layers upon layers
of independent patches soon ceases to be readable or maintainable.
Ideally, once your patch has gone in, someone looking at the code for
the first time wouldn't even suspect it hadn't always been there.

Typical sins in this area include not following the coding style or
commenting style of immediately adjacent code, failing to update
comments that your patch has rendered inaccurate, intentionally making
your edits "stand out", etc. While pg_indent will fix some of the
simpler transgressions, it's not very good with comment style, and
it certainly can't fix failures of omission. (I dislike committing code
that is far away from pg_indent style anyway, since even if it will get
fixed later, I'm still gonna have to look at it until then.)

Sometimes patch authors seem to prefer shorter patches over better
integrated ones --- this particularly happens when the "most natural"
way of adding something would require refactoring existing code.
I understand the reasons for preferring a smaller patch, but you
really need to take the long view about what the code will look like
later.

I guess this is coming off as more advice to patch authors than
reviewers, but it is definitely a big deal in my eyes --- I typically
spend about as much time on issues of this sort as on functional
correctness. And it is something that reviewers can fix if they
care to.

regards, tom lane