Open Stack

On Fri, Aug 22, 2014 at 10:49:51AM +0100, Steven Hardy wrote:
> On Thu, Aug 21, 2014 at 05:05:04PM +0100, Matthew Booth wrote:
> > I also think it's worth explicitly documenting a few things we
> > might/should mention in a review, but which aren't a reason that the
> > project would be better off without it:
> > 
> > * Stylistic issues which are not covered by HACKING
> > 
> > By stylistic, I mean changes which have no functional impact on the code
> > whatsoever. If a purely stylistic issue is sufficiently important to
> > reject code which doesn't adhere to it, it is important enough to add to
> > HACKING.
> 
> I'll sometimes +1 a change if it looks functionally OK but has some
> stylistic or cosmetic issues I would prefer to see fixed before giving a
> +2.  I see that as a "soft" +2, it's not blocking anything, but I'm giving
> the patch owner the chance to fix the problem (which they nearly always
> do).
> 
> Although if a patch contains really significant uglies, I think giving a "I
> would prefer you didn't merge this, in it's current state" with lots of
> constructive comments wrt how to improve things is perfectly reasonable.
> 
> > * I can think of a better way of doing this
> > 
> > There may be a better solution, but there is already an existing
> > solution. We should only be rejecting work that has already been done if
> > it would detract from the project for one of the reasons above. We can
> > always improve it further later if we find the developer time.
> 
> Agreed, although again I'd encourage folks to +1 and leave detailed
> information about how to improve the solution - most people (myself
> included) really appreciate learning better ways to do things.  I've
> definitely become a much better python developer as a result of the
> detailed scrutiny and feedback provided via code reviews.
> 
> So while I agree with the general message you seem to be proposing (e.g
> don't -1 for really trivial stuff), I think it's important to recognise
> that if there are obvious and non-painful ways to improve code-quality, the
> review is the time to do that.

One thing I have seen some people (eg Mark McLoughlin) do a number
of times is to actually submit followup patches. eg they will point
out the minor style issue, or idea for a better approach, but still
leave a +1/+2 score. Then submit a followup change to deal with that
"nitpicking".  This seems like it is quite an effective approach
because it ensures the original authors' work gets through review
more quickly. Using a separate follow-on patch also avoids the idea
of the reviewer hijacking the original contributors patches by editing
them & reposting directly

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|