Open Stack

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.




>
> 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