Hi!
I would like do disagree with some of the points.
First of all, '-1' mark may have a wrong perception especially among new
contributors.
-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.
> Not sure if that works
If you as a reviewer have a gut feeling ('not sure') that it may not work -
put a -1.
Let submitter explain it to you. You can change your score later without
requiring to change the patch.
Also, there are plenty of cases when the code could not be tested because
it requires specific environment to run.
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.
On point #2:
> Let something repeat a couple of times just to be sure it actually is
reusable.
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.
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.
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.
Having said that, I believe that it's not very difficult to get through a
review where you get -1 for code & style issues.
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.
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.
So once a reviewer started a review (put a mark) - it is his responsibility
to work further on this review.
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.
Thanks,
Eugene.
On Wed, Nov 6, 2013 at 11:26 PM, Andrew Woodward <xar...@gmail.com> wrote:
> Great thread and very insightful. Yes; please make this more
> accessible to other reviewers. Adding this to the wiki is a great
> start.
>
> Andrew
> Mirantis
>
> On Wed, Nov 6, 2013 at 11:05 AM, Pitucha, Stanislaw Izaak
> <stanislaw.pitu...@hp.com> wrote:
> > How about putting it on a wiki and linking it from the top bar of gerrit
> itself? (call it "review guidelines" for example)
> > That way, even people who didn't look for it explicitly could find it.
> >
> > Regards,
> > Stanisław Pitucha
> > Cloud Services
> > Hewlett Packard
> >
> > -----Original Message-----
> > From: Sergey Skripnick [mailto:sskripn...@mirantis.com]
> > Sent: Wednesday, November 06, 2013 6:50 PM
> > To: OpenStack Development Mailing List (not for usage questions)
> > Subject: Re: [openstack-dev] Bad review patterns
> >
> >
> > This definitely should be somewhere in wiki or blog and in the bookmarks.
> >
> > _______________________________________________
> > OpenStack-dev mailing list
> > OpenStack-dev@lists.openstack.org
> > http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
> >
> > _______________________________________________
> > OpenStack-dev mailing list
> > OpenStack-dev@lists.openstack.org
> > http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>
>
>
> --
> If google has done it, Google did it right!
>
> _______________________________________________
> OpenStack-dev mailing list
> OpenStack-dev@lists.openstack.org
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>
_______________________________________________
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
