Open Stack

On Thu, Aug 21, 2014 at 05:05:04PM +0100, Matthew Booth wrote:
> "I would prefer that you didn't merge this."
> 
> i.e. The project is better off without it.

I'm not quite sure how you make that translation, I would interpret -2 as
meaning the project would be better off without a change.

FWIW, I've always interpreted "I would prefer that you didn't merge this."
as having an implied suffix of "(in it's current state)".

> 
> This seems to mean different things to different people. There's a list
> here which contains some criteria for new commits:
> 
> https://wiki.openstack.org/wiki/ReviewChecklist.
> 
> There's also a treatise on git commit messages and the structure of a
> commit here:
> 
> https://wiki.openstack.org/wiki/GitCommitMessages
> 
> However, these don't really cover the general case of what a -1 means.
> Here's my brain dump:
> 
> * It contains bugs
> * It is likely to confuse future developers/maintainers
> * It is likely to lead to bugs
> * It is inconsistent with other solutions to similar problems
> * It adds complexity which is not matched by its benefits
> * It isn't flexible enough for future work landing RSN
> * It combines multiple changes in a single commit
> 
> Any more? I'd be happy to update the above wiki page with any consensus.
> It would be useful if any generally accepted criteria were readily
> referenceable.
> 
> 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.

I've been flamed before for saying this, but I maintain that part of the
reason we have so many (mostly new and non-core) reviewers leaving -1
feedback for really trivial stuff is that we collect, publish and in some
cases over-analyse review statistics.

Steve