Open Stack

On Thu, Aug 21, 2014 at 11:21 AM, Daniel P. Berrange <berrange at 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 at lists.openstack.org
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstack.org/pipermail/openstack-dev/attachments/20140821/f38dd88b/attachment.html>