Open Stack

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.