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
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@lists.openstack.org
<mailto: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
_______________________________________________
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
