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