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 <berra...@redhat.com>
wrote:
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
as
"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.
Regards,
Daniel
_______________________________________________
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
