Open Stack

On 08/21/2014 12:34 PM, Dolph Mathews wrote:
>
> On Thu, Aug 21, 2014 at 11:21 AM, Daniel P. Berrange 
> <berrange at redhat.com <mailto: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."

Heh...well, there are a couple other aspects:

1.  I am unsure if my understanding is correct.  I'd like to have some 
validation, and, if I am wrong, I'll withdraw the objections.

2.  If I make the change, I can no longer +2/+A the review.  If you make 
the change, I can approve it.


>
> 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://search.cpan.org/%7Edanberr/> :|
>     |: http://entangle-photo.org      -o- http://live.gnome.org/gtk-vnc :|
>
>     _______________________________________________
>     OpenStack-dev mailing list
>     OpenStack-dev at lists.openstack.org
>     <mailto:OpenStack-dev at lists.openstack.org>
>     http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>
>
>
>
> _______________________________________________
> 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/f5a4f961/attachment.html>