<html><head><meta http-equiv="Content-Type" content="text/html charset=windows-1252"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><br><div><div>On Aug 21, 2014, at 9:42 AM, Adam Young <<a href="mailto:ayoung@redhat.com">ayoung@redhat.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite">
<meta content="text/html; charset=ISO-8859-1" http-equiv="Content-Type">
<div bgcolor="#FFFFFF" text="#000000">
<div class="moz-cite-prefix">On 08/21/2014 12:34 PM, Dolph Mathews
wrote:<br>
</div>
<blockquote cite="mid:CAC=h7gUNPzeEpV3sLJZNVB89t+ptoaZ6YE55c6nmNNiR42W1LQ@mail.gmail.com" type="cite">
<div dir="ltr">
<div class="gmail_extra"><br>
<div class="gmail_quote">On Thu, Aug 21, 2014 at 11:21 AM,
Daniel P. Berrange <span dir="ltr"><<a moz-do-not-send="true" href="mailto:berrange@redhat.com" target="_blank">berrange@redhat.com</a>></span>
wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px
0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div class="">On Thu, Aug 21, 2014 at 05:05:04PM +0100,
Matthew Booth wrote:<br>
> "I would prefer that you didn't merge this."<br>
><br>
> i.e. The project is better off without it.<br>
<br>
</div>
A bit off topic, but I've never liked this message that
gets added<br>
as it think it sounds overly negative. It would better
written<br>
as<br>
<br>
"This patch needs further work before it can be merged"<br>
</blockquote>
<div><br>
</div>
<div>++ "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."</div>
</div>
</div>
</div>
</blockquote>
<br>
Heh...well, there are a couple other aspects:<br>
<br>
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.<br>
<br>
2. If I make the change, I can no longer +2/+A the review. If you
make the change, I can approve it.<br></div></blockquote><br></div><div>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:</div><div><br></div><div>1) commenting “please change if foo==None: to if foo is None:”</div><div>2) waiting for the reviewer to exactly what you suggested</div><div>3) +2 the result</div><div><br></div><div>and:</div><div><br></div><div>1) you change if foo==None: to if foo is None: for the author</div><div>2) +2 the result</div><div><br></div><div>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.</div><div><br></div><div>Vish</div><div><br></div><div><br></div><br></body></html>