On Aug 21, 2014, at 9:42 AM, Adam Young <ayo...@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 <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."
> 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 

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


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.


Attachment: signature.asc
Description: Message signed with OpenPGP using GPGMail

OpenStack-dev mailing list

Reply via email to