On Thu, Aug 21, 2014 at 11:21 AM, Daniel P. Berrange <berra...@redhat.com> 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" > ++ "This patch needs further work before it can be merged, and as a reviewer, I am either too lazy or just unwilling to checkout your patch and fix those issues myself." http://dolphm.com/reviewing-code > 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 > -- > |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ > :| > |: http://libvirt.org -o- http://virt-manager.org > :| > |: http://autobuild.org -o- http://search.cpan.org/~danberr/ > :| > |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc > :| > > _______________________________________________ > OpenStack-dev mailing list > OpenStack-dev@lists.openstack.org > http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev >
_______________________________________________ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev