Comments inline below.
Best Regards, Lance On Thu, Aug 21, 2014 at 11:40 AM, Adam Young <ayo...@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@lists.openstack.org > http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev >
_______________________________________________ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev