Open Stack

On Aug 21, 2014, at 9:42 AM, Adam Young <ayoung at redhat.com> wrote:

> On 08/21/2014 12:34 PM, Dolph Mathews wrote:
>> 
>> On Thu, Aug 21, 2014 at 11:21 AM, Daniel P. Berrange <berrange at 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."
> 
> Heh...well, there are a couple other aspects:
> 
> 1.  I am unsure if my understanding is correct.  I'd like to have some validation, and, if I am wrong, I'll withdraw the objections.
> 
> 2.  If I make the change, I can no longer +2/+A the review.  If you make the change, I can approve it.

I don’t think this is correct. I’m totally ok with a core reviewer making a minor change to a patch AND +2ing it. This is especially true of minor things like spelling issues or code cleanliness. The only real functional difference between:

1) commenting “please change if foo==None: to if foo is None:”
2) waiting for the reviewer to exactly what you suggested
3) +2 the result

and:

1) you change if foo==None: to if foo is None: for the author
2) +2 the result

is the second set is WAY faster. Of course this only applies to minor changes. If you are refactoring more significantly it is nice to get the original poster’s feedback so the first option might be better.

Vish



-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstack.org/pipermail/openstack-dev/attachments/20140821/e983bf3c/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 455 bytes
Desc: Message signed with OpenPGP using GPGMail
URL: <http://lists.openstack.org/pipermail/openstack-dev/attachments/20140821/e983bf3c/attachment.pgp>