<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 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><br></div><div><a 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 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 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 href="http://berrange.com" target="_blank">http://berrange.com</a>      -o-    <a href="http://www.flickr.com/photos/dberrange/" target="_blank">http://www.flickr.com/photos/dberrange/</a> :|<br>
|: <a href="http://libvirt.org" target="_blank">http://libvirt.org</a>              -o-             <a href="http://virt-manager.org" target="_blank">http://virt-manager.org</a> :|<br>
|: <a href="http://autobuild.org" target="_blank">http://autobuild.org</a>       -o-         <a href="http://search.cpan.org/~danberr/" target="_blank">http://search.cpan.org/~danberr/</a> :|<br>
|: <a href="http://entangle-photo.org" target="_blank">http://entangle-photo.org</a>       -o-       <a 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 href="mailto:OpenStack-dev@lists.openstack.org">OpenStack-dev@lists.openstack.org</a><br>
<a 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>