On Fri, 2019-01-11 at 15:39 -0600, Ben Nemec wrote:
On 1/10/19 5:03 PM, Sean Mooney wrote:
On Thu, 2019-01-10 at 13:42 -0600, Sean McGinnis wrote:
I don't know if this was the reasoning behind Cinder's system, but I know some people object to procedural -2 because it's a big hammer to essentially say "not right now". It overloads the meaning of the vote in a potentially confusing way that requires explanation every time it's used. At least I hope procedural -2's always include a comment.
This was exactly the reasoning. -2 is overloaded, but its primary meaning was/is "we do not want this code change". It just happens that it was also a convenient way to say that with "right now" at the end.
The Review-Priority -1 is a clear way to say whether something is held because it can't be merged right now due to procedural or process reasons, versus something that we just don't want at all.
for what its worth my understanding of why a procdural -2 is more correct is that this change cannot be merged because it has not met the procedual requirement to be considerd for this release. haveing received several over the years i have never seen it to carry any malaise or weight then the zuul pep8 job complianing about the line lenght of my code. with either a procedural -2 or a verify -1 from zuul my code is equally un mergeable.
the prime example being a patch that requires a spec that has not been approved. while most cores will not approve chage when other cores have left a -1 mistakes happen and the -2 does emphasise the point that even if the code is perfect under the porject processes this change should not be acitvly reporposed until the issue raised by the -2 has been addressed. In the case of a procedual -2 that typically means the spec is merge or the master branch opens for the next cycle.
i agree that procedural -2's can seam harsh at first glance but i have also never seen one left without a comment explaining why it was left. the issue with a procedural -1 is i can jsut resubmit the patch several times and it can get lost in the comments.
I don't think that's a problem with this new field. It sounds like priority -1 carries over from PS to PS.
we recently intoduced a new review priority lable if we really wanted to disabiguate form normal -2s then we coudl have an explcitly lable for it but i personally would prefer to keep procedural -2s.
To be clear, I have both used and received procedural -2's as well and they don't particularly bother me, but I can see where if you were someone who was new to the community or just a part-time contributor not as familiar with our processes it might be an unpleasant experience to see that -2 show up. As I said, I don't know that I would advocate for this on the basis of replacing procedural -2 alone, but if we're adding the category anyway I mildly prefer using it for procedural blockers in the future.
i think i partally missunderstood the proposal. i had parsed it as replacing procedual code review -2 with a code review -1 rahter then review priority -1. if all projects adopt review priorty going forward then that might makes sense for those that dont i think code review -2 still makes sense.