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 :| _______________________________________________ OpenStack-dev mailing list OpenStackfirstname.lastname@example.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev