<div dir="ltr">Comments inline below.<div><br></div><div><br></div><div>Best Regards, </div><div>Lance<br><div class="gmail_extra"><br><br><div class="gmail_quote">On Thu, Aug 21, 2014 at 11:40 AM, Adam Young <span dir="ltr"><<a href="mailto:ayoung@redhat.com" target="_blank">ayoung@redhat.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="">On 08/21/2014 12:21 PM, Daniel P. Berrange wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
On Thu, Aug 21, 2014 at 05:05:04PM +0100, Matthew Booth wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
"I would prefer that you didn't merge this."<br>
<br>
i.e. The project is better off without it.<br>
</blockquote>
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>
Excellent.<br>
<br>
It also bothers me that a -1 is dropped upon a new submission of the patch. It seems to me that the review should instead indicate on a given line level comment whether it is grounds for -1. If it is then either that same reviewer or another can decide whether a given fix address the reviewers request.<br>
<br>
<br>
As a core reviewer, I have the power to -2 something. That is considered a "do not follow this approach" message today. I rarely exercise it, even for changes that I consider essential. One reason is that a review with a -2 on it won't get additional reviews, and that is not my intention.<div class="HOEnZb">
<div class="h5"><br>
<br></div></div></blockquote><div> </div><div>In this case, one way we can try to avoid the 'negativity' of a -2 is to suggest the committer to mark the patch as Workflow -1 (WIP), and encourage them to air out their explanation in project meeting open discussion and IRC. To me, this changes the idea from "this patch is going in a separate direction than the project" to "this patch/idea hasn't been shot down, but the committer needs a little help fleshing it out". </div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">
<br>
<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
as that gives a positive expectation that the work is still<br>
wanted by the project in general<br>
<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
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/<u></u>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/<u></u>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>
</blockquote>
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>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
* 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>
</blockquote>
It is always worth improving our docs to give more guidance like<br>
ths.<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
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>
</blockquote>
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>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
* 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>
</blockquote>
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>
</blockquote>
<br>
<br></div></div><div class="HOEnZb"><div class="h5">
______________________________<u></u>_________________<br>
OpenStack-dev mailing list<br>
<a href="mailto:OpenStack-dev@lists.openstack.org" target="_blank">OpenStack-dev@lists.openstack.<u></u>org</a><br>
<a href="http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev" target="_blank">http://lists.openstack.org/<u></u>cgi-bin/mailman/listinfo/<u></u>openstack-dev</a><br>
</div></div></blockquote></div><br></div></div></div>