[openstack-dev] Criteria for giving a -1 in a review
ayoung at redhat.com
Thu Aug 21 17:23:05 UTC 2014
On 08/21/2014 12:53 PM, Daniel P. Berrange wrote:
> On Thu, Aug 21, 2014 at 11:34:48AM -0500, Dolph Mathews wrote:
>> On Thu, Aug 21, 2014 at 11:21 AM, Daniel P. Berrange <berrange at redhat.com>
>>> On Thu, Aug 21, 2014 at 05:05:04PM +0100, Matthew Booth wrote:
>>>> "I would prefer that you didn't merge this."
>>>> i.e. The project is better off without it.
>>> A bit off topic, but I've never liked this message that gets added
>>> as it think it sounds overly negative. It would better written
>>> "This patch needs further work before it can be merged"
>> ++ "This patch needs further work before it can be merged, and as a
>> reviewer, I am either too lazy or just unwilling to checkout your patch and
>> fix those issues myself."
> I find the suggestion that reviewers are either "too lazy" or "unwilling"
> to fix it themselves rather distasteful to be honest.
That was from the Keystone PTL. I think he was going for "vaguely self
deprecating" as opposed to dissing the reviewers.
> It is certainly valid for a code reviewer to fix an issue themselves &
> re-post the patch, but that is not something to encourage a general day
> to day practice for a number of reasons.
> - When there are multiple people reviewing it would quickly become a
> mess of conflicts if each & every reviewer took it upon themselves
> to rework & repost the patch.
> - The original submitter should generally always have the chance to
> rebut any feedback from reviewers, since reviewers are not infallible,
> nor always aware of the bigger picture or as familiar with the code
> being changed.
> - When a patch is a small part of a larger series, it can be a very
> disruptive if someone else takes it, changes it & resubmits it,
> as that invalidates all following patches in a series in gerrit.
> - It does not scale to have reviewers take on much of the burden of
> actually writing the fixes, running the tests & resubmitting.
> - Having the original author deal with the review feedback actually
> helps that contributor learn new things, so that they will be able
> to do a better job for future patches they contribute
> I'd only recommend fixing & resubmitting someone else's patch if it is
> a really trivial thing that needed tweaking before approval for merge,
> or if they are known to be away for a prolonged time and the patch was
> blocking other important pending work.
More information about the OpenStack-dev