<div dir="ltr">Hi!<div><br></div><div>I would like do disagree with some of the points.</div><div>First of all, '-1' mark may have a wrong perception especially among new contributors. </div><div>-1 doesn't mean reviewers don't want your code (which is -2), it means they either not sure it is good enough or they see how you could make it better.</div>
<div><br></div><div>> <span style="font-family:arial,sans-serif;font-size:13px">Not sure if that works</span></div><div><span style="font-family:arial,sans-serif;font-size:13px">If you as a reviewer have a gut feeling ('not sure') that it may not work - put a -1.</span></div>
<div><span style="font-family:arial,sans-serif;font-size:13px">Let submitter explain it to you. You can change your score later without requiring to change the patch.</span></div><div><span style="font-family:arial,sans-serif;font-size:13px">Also, there are plenty of cases when the code could not be tested because it requires specific environment to run.</span></div>
<div><font face="arial, sans-serif">I think it's ok to put -1 in case of uncertainty. The only thing you should care about is that such -1 is not 'finished' because you don't require submitter to change anything, so you need to check if your concerns were addressed and potentially change the score.</font></div>
<div><font face="arial, sans-serif"><br></font></div><div><font face="arial, sans-serif">On point #2:</font></div><div><font face="arial, sans-serif">> </font><span style="font-family:arial,sans-serif;font-size:13px"> </span><span style="font-family:arial,sans-serif;font-size:13px">Let something </span><span style="font-family:arial,sans-serif;font-size:13px">repeat a couple of times just to be sure it actually is reusable.</span></div>
<div><span style="font-family:arial,sans-serif;font-size:13px">I think code repetition is not desirable in most of the cases and only occasionally repeating code gets merged. That happens especially when someone didn't put reusable code in a common place so others unaware of its existance. Instead of doing refactoring later on (who will do this?) just rely on reviewer's opinion and check if generalization could be done.</span></div>
<div><font face="arial, sans-serif">And this can grow as a snowball (oh, he has copy-pasted his stuff, why shouldn't I do the same?) so the earlier it is caught - the better.</font></div><div><span style="font-family:arial,sans-serif;font-size:13px"><br>
</span></div><div><span style="font-family:arial,sans-serif;font-size:13px">On other points: the patch is required to be self-consistent. It needs to solve something that is stated in the bug/commit message and no more (and hopefully not less) than that. So the scope really matters. I haven't see much comments from reviewers requesting to make occasional fixes. Although sometimes it make sense to ask for such, especially if they are related, reasonable and the patch is going to be improved anyways (due to some other issues). It also may happen that submitter has missed certain things that also fall in the scope of the work he is doing.</span></div>
<div><span style="font-family:arial,sans-serif;font-size:13px"><br></span></div><div><span style="font-family:arial,sans-serif;font-size:13px">Having said that, I believe that it's not very difficult to get through a review where you get -1 for code & style issues. </span></div>
<div><span style="font-family:arial,sans-serif;font-size:13px">When you have your code, IDE, git and gerrit at your hands it is a matter of minutes (hours rarely) to resolve those and resubmit.</span></div><div><span style="font-family:arial,sans-serif;font-size:13px"><br>
</span></div><div><font face="arial, sans-serif">The real issue with the review process is that reviewers should back up his mark once they has put it and they not usually do that for various reasons. The worst thing to do is to put -1 and go away without leaving a chance for submitter to explain what he meant and wanted. </font></div>
<div><font face="arial, sans-serif">So once a reviewer started a review (put a mark) - it is his responsibility to work further on this review.</font></div><div><font face="arial, sans-serif">Persistent -1 should indicate that reviewer and submitter could not agree on fundamentals of the patch (design, workflow, etc), which, in turn, means, that a discussion should be held within a community before the work on the patch is continued.</font></div>
<div><font face="arial, sans-serif"><br></font></div><div><font face="arial, sans-serif">Thanks,</font></div><div><font face="arial, sans-serif">Eugene.</font></div><div><font face="arial, sans-serif"><br></font></div><div>
<font face="arial, sans-serif"><br></font></div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Wed, Nov 6, 2013 at 11:26 PM, Andrew Woodward <span dir="ltr"><<a href="mailto:xarses@gmail.com" target="_blank">xarses@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Great thread and very insightful. Yes; please make this more<br>
accessible to other reviewers. Adding this to the wiki is a great<br>
start.<br>
<br>
Andrew<br>
Mirantis<br>
<div class="HOEnZb"><div class="h5"><br>
On Wed, Nov 6, 2013 at 11:05 AM, Pitucha, Stanislaw Izaak<br>
<<a href="mailto:stanislaw.pitucha@hp.com">stanislaw.pitucha@hp.com</a>> wrote:<br>
> How about putting it on a wiki and linking it from the top bar of gerrit itself? (call it "review guidelines" for example)<br>
> That way, even people who didn't look for it explicitly could find it.<br>
><br>
> Regards,<br>
> Stanisław Pitucha<br>
> Cloud Services<br>
> Hewlett Packard<br>
><br>
> -----Original Message-----<br>
> From: Sergey Skripnick [mailto:<a href="mailto:sskripnick@mirantis.com">sskripnick@mirantis.com</a>]<br>
> Sent: Wednesday, November 06, 2013 6:50 PM<br>
> To: OpenStack Development Mailing List (not for usage questions)<br>
> Subject: Re: [openstack-dev] Bad review patterns<br>
><br>
><br>
> This definitely should be somewhere in wiki or blog and in the bookmarks.<br>
><br>
> _______________________________________________<br>
> OpenStack-dev mailing list<br>
> <a href="mailto:OpenStack-dev@lists.openstack.org">OpenStack-dev@lists.openstack.org</a><br>
> <a href="http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev" target="_blank">http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev</a><br>
><br>
> _______________________________________________<br>
> OpenStack-dev mailing list<br>
> <a href="mailto:OpenStack-dev@lists.openstack.org">OpenStack-dev@lists.openstack.org</a><br>
> <a href="http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev" target="_blank">http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev</a><br>
<br>
<br>
<br>
</div></div><span class="HOEnZb"><font color="#888888">--<br>
If google has done it, Google did it right!<br>
</font></span><div class="HOEnZb"><div class="h5"><br>
_______________________________________________<br>
OpenStack-dev mailing list<br>
<a href="mailto:OpenStack-dev@lists.openstack.org">OpenStack-dev@lists.openstack.org</a><br>
<a href="http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev" target="_blank">http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev</a><br>
</div></div></blockquote></div><br></div>