On 21.01.2013 02:07, Jeff Janes wrote:
As a junior reviewer, I'd like to know if my main task should be to decide
between 1) writing a review convincing you or Tom that your judgement is
hasty, or 2) to convince the author that your judgement is correct. That
would provide me with some direction, rather than just having me pondering
whether a certain variable name ought to have one more or one fewer
underscores in it. On the other hand if a summary judgement is that the
patch is fundamentally sound but needs some line-by-line code-lawyering, or
some performance testing, or documentation/usability testing, or needs to
ponder the implications to part XYZ of the code base (which neither I nor
the other have even heard of before), that would also be good to know.
My desire is not for you to tell me what the outcome of the review should
be, but rather what the focus of it should be.
Often a patch contains a contentious change buried deep in the patch.
The patch submitter might not realize there's anything out of the
ordinary in changing parser behavior based on a GUC, or doing things in
the executor that should be done in the planner, so he doesn't mention
it in the submission email or highlight it with any comments. The longer
the patch, the easier it is for things like that to hide in the crowd.
If a reviewer can bring those potentially contentious things that don't
"smell right" to light, that's extremely helpful. It's not so important
what judgment you make on it; just bringing it up, in a short, separate
reply to the email thread, will allow the submitter to reconsider, and a
committer to jump in early to say "yeah, you can't do that, because X.".
IMHO that's the single most important task of a review.
- Heikki
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
