Open Stack

Comments inline below.


Best Regards,
Lance


On Thu, Aug 21, 2014 at 11:40 AM, Adam Young <ayoung at redhat.com> wrote:

> On 08/21/2014 12:21 PM, Daniel P. Berrange 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"
>>
> Excellent.
>
> 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.
>
>
> 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.
>
>
>
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".


>
>
>
>> 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
>>
>
>
> _______________________________________________
> OpenStack-dev mailing list
> OpenStack-dev at lists.openstack.org
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstack.org/pipermail/openstack-dev/attachments/20140821/db34566a/attachment.html>