spritejs
spritejs copied to clipboard
spritejs
Don't allow merge PRs untils all conversations are resolved and all reviewers reviewed changes
I think it's more strict than just blocking merging by request changes and it works automatically. So we don't depend on how reviewers review PRs and when they actually request changes. :)
These rules must be applied to admins too, I think.
Isn't this just a given? Letting reviewers review changes and making sure every message is resolved.
I don't think anything needs to be changed in the settings regarding making reviews stale, maybe a small sentence in the maintainers guide about making sure all comments are resolved, although I think it already exists (but I couldn't find it).
Isn't this just a given? Letting reviewers review changes and making sure every message is resolved.
Reviews may make sure every message is resolved but it's not guaranteed. That's why I suggest to guarantee conversion resolution by GitHub and not to rely on ourselves. We are humans and can make mistakes.
Sure, we don't want to merge until all discussions are resolved. However, sometimes it is advantageous to leave a discussion thread as "unresolved" in the GitHub interface. For example, it could be an important discussion is resolved, but we don't want to mark it resolved, because that would minimise it - and we want to draw attention to it.
This is I guess a question of etiquette. If a discussion is unresolved for a very long time (e.g. 1yr+ with multiple reminders sent) and both the other people are inactive AND all current maintainers are in agreement over a solution, then it may be advantageous to go ahead anyway.
It is for this reason we have a bot to auto-close stale PRs, I suppose.
I am doubt about that important discussions must be in GitHub PRs. What about to create issues for such things? 🤔