How to handle merge of new features

[h-mayorquin]

This is related to #1707 and is an issue we have encountered more than once. Something was merged or some feedback was ignored.

What about the following proposal:
If someone think that a PR is dangerous or bad then they can block it with request changes in the review window or indicating in a comment that more review is coming. Even better if we can write a small one or two sentences about what the concern is.

But for stuff that could have a better implementation but is mostly fine like #1707 it seems that an extra issue should suffice. Having something sub-optimal (if it is not dangerous) is better than having nothing. Of course we can discuss where a proposal lies within the axis of dangerous-to-bad-idea-to-good-to-optimal but I think that that discussion can be good on itself.

How do you feel about this? For example, for issue #1707 I myself wanted to read more about the standards fo casting and make some good feedback based on that but I don't have time now and did not want to delay @alejoe91 with this.

The request for more time is coming from a good place as "more eyes is good" and we should be aware that we are all busy and have limited resources to adress issues. That said, we need to make a trade-off between this and other factors like:

• Motivation and sense of progress for PRs is good. People have limited bandwith and the ideas can be forgotten. Motivation is a limited resource and timely feedback helps to keep a sense of momentum.
• Issues might be blocking something else.
• Other fixes can get the way making merging and reasoning dificult. Coordination problems.

We don't need to come with super-precise rules for doing this. But having a sense of agrement should reduce frictions and misunderstandings between us.

[alejoe91]

Thank you @h-mayorquin

I agree that it would be hard to set universal rules! For #1707 I may have merged to fast. Maybe what we can do is to announce that a certain PR (after approval by at least one person), will be open for another 24-48 hours and tag the other devs to request additional comments within that timeframe. We could maybe even add a label "ready-to-merge" so that they stick out in the PR list. What do you think?

[h-mayorquin]

A tag of ready to merge seems good to me. We also have the option of request changes to block merge I think?

[h-mayorquin]

I find interesting that even python core is discussing stuff like this:
https://discuss.python.org/t/sc-poll-should-we-require-reviews-to-merge-all-prs/29410

[zm711]

Maybe now that there are more people it would be good to bring back this discussion?

[JoeZiminski]

I think it's useful to have a general procedure for such things outlined in the contributing guidelines. It's doesn't have to be a strict rule but a useful recommendation. Something like if a review is requested and approved by at least one reviewer, but other reviewers have not responded, tag / slack them and give 24-48h to respond. After this the PR can be merged without further review. I think this approach is preferable to a label on the PR because if someone is very busy they will probably miss the labels / not link up mentally that it is a label on a PR they has been requested to review. So I think a direct github tag or slack message would be more effective.

This is a slight digression but I was thinking of opening an issue suggesting a PR template message, that includes a few prompts (e.g. what is this PR about) and reminders (are there tests / documentation supporting new features?). Often I am pursuing the PRs out of interest but there is not enough information in the PR message to understand its purpose. Something like this could be added as a checkbox ('Any unresponsive reviewers tagged with 48h notice'). If people are not averse to the PR template idea I will open a new issue.