On 08/21/2014 12:34 PM, Dolph Mathews wrote:

On Thu, Aug 21, 2014 at 11:21 AM, Daniel P. Berrange <berra...@redhat.com <mailto: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

      "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.


    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
    > 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


    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
    > It would be useful if any generally accepted criteria were readily
    > referenceable.

    It is always worth improving our docs to give more guidance like

    > 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
    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
    mitigated right away.

    |: 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 mailing list

OpenStack-dev mailing list

Reply via email to