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 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
signature.asc
Description: Message signed with OpenPGP using GPGMail
_______________________________________________ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
