Do you mean as part of a blocking review, or just in general?
I don’t honestly understand the focus on ninja fixes or rebasing, in either context. Why would a project rebase its work until ready to merge? Why would you worry that a team well resourced with experienced contributors (30+yrs between them) don’t know how to work correctly with ninja fixes? These are all minor details, surely?
I understand your concern around flaky tests, particularly since it seems other work has let some slip through. I don’t believe this is a blocking review concern, as it represents its own blocking requirement. I believe I have responded positively to this query already though? On 31 Jan 2023, at 01:46, Ekaterina Dimitrova <e.dimitr...@gmail.com> wrote:
Benedict, Is it an issue to ask whether flaky tests will be addressed as per our project agreement? Or about ninja fixes and why a branch was not rebased during development? What did I miss?
By the way I do not ask these questions to block anyone, even suggested to help with CI…it’s a pity this part was dismissed…
I can see that Caleb is handling the things around the merge ticket with high attention to the details as always. I can only thank him!
At this point I see this thread mostly as - this is the first feature branch since quite some time, people are unsure about certain things - let’s see how we handle this for the next time to be more efficient and clear. I think you already took some actions in your suggestion earlier today with the document around comments.
Review should primarily ask: "is this correct?" and "could this be done differently (clearer, faster, more correct, etc)?" Blocking reviews especially, because why else would a reasonable contributor want to block progress?
These questions can of course be asked of implementation details for any CEP.
I have said before, a proposal to conduct a blocking review of this kind - if very late in my view - would be valid, though timeline would have to be debated.
Reviewers with weaker aspirations have plenty of time available to them - two weeks have already passed, and another couple will likely yet (there isn't a rush). But it is novel to propose that such optional reviews be treated as blocking.
Ooops, I missed copy pasting this reply into my previous email: I'm realizing in retrospect this leaves ambiguity
Another misreading at least of the intent of these clauses, is that they were to ensure that concerns about a design and approach are listened to, and addressed to the satisfaction of interested parties. It was essentially codifying the project’s long term etiquette around pieces of work with either competing proposals or fundamental concerns. It has historically helped to avoid escalation to vetoes, or reverting code after commit.
It wasn’t intended that any reason might be invoked, as seems to have been inferred, and perhaps this should be clarified, though I had hoped it would be captured by the word “reasonable". Minor concerns that haven’t been caught by the initial review process can always be addressed in follow-up work, as is very common.
Wouldn't you expect such concerns to at least partially now have been covered in the CEP discussion, up front? I would expect at most at this stage someone could validate that the implementation follows the CEP. But I wouldn't expect a debate on competing approaches at this stage.
henrik --
|
