<html>
<head>
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<div class="moz-cite-prefix">On 08/21/2014 12:34 PM, Dolph Mathews
wrote:<br>
</div>
<blockquote
cite="mid:CAC=h7gUNPzeEpV3sLJZNVB89t+ptoaZ6YE55c6nmNNiR42W1LQ@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_extra"><br>
<div class="gmail_quote">On Thu, Aug 21, 2014 at 11:21 AM,
Daniel P. Berrange <span dir="ltr"><<a
moz-do-not-send="true" href="mailto:berrange@redhat.com"
target="_blank">berrange@redhat.com</a>></span>
wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px
0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div class="">On Thu, Aug 21, 2014 at 05:05:04PM +0100,
Matthew Booth wrote:<br>
> "I would prefer that you didn't merge this."<br>
><br>
> i.e. The project is better off without it.<br>
<br>
</div>
A bit off topic, but I've never liked this message that
gets added<br>
as it think it sounds overly negative. It would better
written<br>
as<br>
<br>
"This patch needs further work before it can be merged"<br>
</blockquote>
<div><br>
</div>
<div>++ "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."</div>
</div>
</div>
</div>
</blockquote>
<br>
Heh...well, there are a couple other aspects:<br>
<br>
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.<br>
<br>
2. If I make the change, I can no longer +2/+A the review. If you
make the change, I can approve it.<br>
<br>
<br>
<blockquote
cite="mid:CAC=h7gUNPzeEpV3sLJZNVB89t+ptoaZ6YE55c6nmNNiR42W1LQ@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<div><br>
</div>
<div><a moz-do-not-send="true"
href="http://dolphm.com/reviewing-code">http://dolphm.com/reviewing-code</a><br>
</div>
<div><br>
</div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px
0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><br>
as that gives a positive expectation that the work is
still<br>
wanted by the project in general<br>
<div class=""><br>
<br>
> This seems to mean different things to different
people. There's a list<br>
> here which contains some criteria for new commits:<br>
><br>
> <a moz-do-not-send="true"
href="https://wiki.openstack.org/wiki/ReviewChecklist"
target="_blank">https://wiki.openstack.org/wiki/ReviewChecklist</a>.<br>
><br>
> There's also a treatise on git commit messages and
the structure of a<br>
> commit here:<br>
><br>
> <a moz-do-not-send="true"
href="https://wiki.openstack.org/wiki/GitCommitMessages"
target="_blank">https://wiki.openstack.org/wiki/GitCommitMessages</a><br>
><br>
> However, these don't really cover the general case
of what a -1 means.<br>
> Here's my brain dump:<br>
><br>
> * It contains bugs<br>
> * It is likely to confuse future
developers/maintainers<br>
> * It is likely to lead to bugs<br>
> * It is inconsistent with other solutions to
similar problems<br>
> * It adds complexity which is not matched by its
benefits<br>
> * It isn't flexible enough for future work landing
RSN<br>
<br>
</div>
s/RSN//<br>
<br>
There are times where the design is not flexible enough
and we<br>
do not want to accept regardless of when future work might
land.<br>
This is specifically the case with things that are adding
APIs<br>
or impacting the RPC protocol. For example proposals for
new<br>
virt driver methods that can't possibly work with other
virt<br>
drivers in the future and would involve incompatible RPC
changes<br>
to fix it.<br>
<div class=""><br>
> * It combines multiple changes in a single commit<br>
><br>
> Any more? I'd be happy to update the above wiki
page with any consensus.<br>
> It would be useful if any generally accepted
criteria were readily<br>
> referenceable.<br>
<br>
</div>
It is always worth improving our docs to give more
guidance like<br>
ths.<br>
<div class=""><br>
> I also think it's worth explicitly documenting a
few things we<br>
> might/should mention in a review, but which aren't
a reason that the<br>
> project would be better off without it:<br>
><br>
> * Stylistic issues which are not covered by HACKING<br>
><br>
> By stylistic, I mean changes which have no
functional impact on the code<br>
> whatsoever. If a purely stylistic issue is
sufficiently important to<br>
> reject code which doesn't adhere to it, it is
important enough to add to<br>
> HACKING.<br>
<br>
</div>
Broadly speaking I agree with the idea that style cleanups
should<br>
have corresponding hacking rules validated automatically.
If some<br>
one proposes a style cleanup which isn't validated I'll
typically<br>
request that they write a hacking check to do so. There
might be<br>
some cases where it isn't practical to validate the rule
automatically<br>
which are none the less worthwhile -1'ing for - these
should be the<br>
exception though. In general we shouldn't do style
cleanups that we<br>
can not automatically validate in some way.<br>
<div class=""><br>
> * I can think of a better way of doing this<br>
><br>
> There may be a better solution, but there is
already an existing<br>
> solution. We should only be rejecting work that has
already been done if<br>
> it would detract from the project for one of the
reasons above. We can<br>
> always improve it further later if we find the
developer time.<br>
><br>
> * It isn't flexible enough for any conceivable
future feature<br>
><br>
> Lets avoid premature generalisation. We can always
generalise as part of<br>
> landing the future feature.<br>
<br>
</div>
See my note about - it isn't always just about premature
generalization<br>
per se, but rather seeing things we are just clearly
written from too<br>
narrow a POV and will cause us pain down the line which
could be easily<br>
mitigated right away.<br>
<br>
Regards,<br>
Daniel<br>
<span class=""><font color="#888888">--<br>
|: <a moz-do-not-send="true"
href="http://berrange.com" target="_blank">http://berrange.com</a>
-o- <a moz-do-not-send="true"
href="http://www.flickr.com/photos/dberrange/"
target="_blank">http://www.flickr.com/photos/dberrange/</a>
:|<br>
|: <a moz-do-not-send="true"
href="http://libvirt.org" target="_blank">http://libvirt.org</a>
-o- <a moz-do-not-send="true"
href="http://virt-manager.org" target="_blank">http://virt-manager.org</a>
:|<br>
|: <a moz-do-not-send="true"
href="http://autobuild.org" target="_blank">http://autobuild.org</a>
-o- <a moz-do-not-send="true"
href="http://search.cpan.org/%7Edanberr/"
target="_blank">http://search.cpan.org/~danberr/</a>
:|<br>
|: <a moz-do-not-send="true"
href="http://entangle-photo.org" target="_blank">http://entangle-photo.org</a>
-o- <a moz-do-not-send="true"
href="http://live.gnome.org/gtk-vnc" target="_blank">http://live.gnome.org/gtk-vnc</a>
:|<br>
</font></span>
<div class="">
<div class="h5"><br>
_______________________________________________<br>
OpenStack-dev mailing list<br>
<a moz-do-not-send="true"
href="mailto:OpenStack-dev@lists.openstack.org">OpenStack-dev@lists.openstack.org</a><br>
<a moz-do-not-send="true"
href="http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev"
target="_blank">http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev</a><br>
</div>
</div>
</blockquote>
</div>
<br>
</div>
</div>
<br>
<fieldset class="mimeAttachmentHeader"></fieldset>
<br>
<pre wrap="">_______________________________________________
OpenStack-dev mailing list
<a class="moz-txt-link-abbreviated" href="mailto:OpenStack-dev@lists.openstack.org">OpenStack-dev@lists.openstack.org</a>
<a class="moz-txt-link-freetext" href="http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev">http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev</a>
</pre>
</blockquote>
<br>
</body>
</html>