On Fri, 2014-08-22 at 11:01 +0100, Daniel P. Berrange wrote: > 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
That's definitely the intent, but I've sometimes regretted it when another reviewer comes along with even *more* trivial concerns and -1s my +2 (another stat we track and over-analyze), and then the submitter fixes these new concerns but doesn't include my fixups, leaving me with a patch to rebase ... at which point you just want to cry :) That lesson learned, I'd now tend to only take this approach with a patch I'm about to +2 and approve. Mark. _______________________________________________ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
