On 08/21/2014 12:21 PM, Daniel P. Berrange wrote:
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.
A bit off topic, but I've never liked this message that gets added
as it think it sounds overly negative. It would better written
as
"This patch needs further work before it can be merged"
Excellent.
It also bothers me that a -1 is dropped upon a new submission of the
patch. It seems to me that the review should instead indicate on a
given line level comment whether it is grounds for -1. If it is then
either that same reviewer or another can decide whether a given fix
address the reviewers request.
As a core reviewer, I have the power to -2 something. That is
considered a "do not follow this approach" message today. I rarely
exercise it, even for changes that I consider essential. One reason is
that a review with a -2 on it won't get additional reviews, and that is
not my intention.
as that gives a positive expectation that the work is still
wanted by the project in general
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
s/RSN//
There are times where the design is not flexible enough and we
do not want to accept regardless of when future work might land.
This is specifically the case with things that are adding APIs
or impacting the RPC protocol. For example proposals for new
virt driver methods that can't possibly work with other virt
drivers in the future and would involve incompatible RPC changes
to fix it.
* 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.
It is always worth improving our docs to give more guidance like
ths.
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.
Broadly speaking I agree with the idea that style cleanups should
have corresponding hacking rules validated automatically. If some
one proposes a style cleanup which isn't validated I'll typically
request that they write a hacking check to do so. There might be
some cases where it isn't practical to validate the rule automatically
which are none the less worthwhile -1'ing for - these should be the
exception though. In general we shouldn't do style cleanups that we
can not automatically validate in some way.
* 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.
* It isn't flexible enough for any conceivable future feature
Lets avoid premature generalisation. We can always generalise as part of
landing the future feature.
See my note about - it isn't always just about premature generalization
per se, but rather seeing things we are just clearly written from too
narrow a POV and will cause us pain down the line which could be easily
mitigated right away.
Regards,
Daniel
_______________________________________________
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
