Criteria for *which* comments "require resolution" could use adjustment.

[chris-martin]

Select Topic Area

Product Feedback

Body

My organization turned on the "require comment resolution" rule because we wanted to help make sure comments don't go noticed.

• Making sure comments are seen is always desired, but it's specifically worth nothing how important it is for auto-merging. Approve-with-suggestions is a very common review outcome for us, as we usually want to leave it up to the change author to decide whether the suggestions are worth implementing, and if so, whether they are worth implementing immediately or would be better pushed off to a separate task in a tech debt queue. Required comment resolution makes it possible to use auto-merge more safely, since it prevents an approve-with-comments review from triggering a merge before the author has had a chance to look at the review.

After living with this rule for a few months, we have some complaints with it:

• We often "review" our own work, leaving comments on things that are particularly noteworthy for reviewers to look at. It is useful to leave these comments present after merging, as part of the historical record. "Resolving" an author's comments on their own work is pretty much always a pointless effort, it hides useful information. It also defeats the purpose of trying to make auto-merge usable, since these comments will need to be left open until after the second-party reviewer leaves their review.

• Not all comments actually require resolution! They only require resolution if they are attached to a code position, which strikes us all as a very arbitrary distinction. If you write your comments under the Files Changed tab, they require acknowledgement before merging; if you write them under the Conversation tab or in the overall Approve review comment, they don't! Feedback in that form is still very easy to miss.

What is missing in my opinion is some ability for the comment author to designate a comment as one that should be acknowledged before merge or not, and the location of comment should be irrelevant.

[github-actions[bot]]

💬 Your Product Feedback Has Been Submitted 🎉

Thank you for taking the time to share your insights with us! Your feedback is invaluable as we build a better GitHub experience for all our users.

Here's what you can expect moving forward ⏩

• Your input will be carefully reviewed and cataloged by members of our product teams. 
  • Due to the high volume of submissions, we may not always be able to provide individual responses.
  • Rest assured, your feedback will help chart our course for product improvements.
• Other users may engage with your post, sharing their own perspectives or experiences. 
• GitHub staff may reach out for further clarification or insight. 
  • We may 'Answer' your discussion if there is a current solution, workaround, or roadmap/changelog post related to the feedback.

Where to look to see what's shipping 👀

• Read the Changelog for real-time updates on the latest GitHub features, enhancements, and calls for feedback.
• Explore our Product Roadmap, which details upcoming major releases and initiatives.

What you can do in the meantime 💻

• Upvote and comment on other user feedback Discussions that resonate with you.
• Add more information at any point! Useful details include: use cases, relevant labels, desired outcomes, and any accompanying screenshots.

As a member of the GitHub community, your participation is essential. While we can't promise that every suggestion will be implemented, we want to emphasize that your feedback is instrumental in guiding our decisions and priorities.

Thank you once again for your contribution to making GitHub even better! We're grateful for your ongoing support and collaboration in shaping the future of our platform. ⭐

[tm-wt-1316]

Coming from AzDO where both file-level and general comments require resolution, this would be a welcome change. I would prefer it be the default to include both. In AzDO when we made a general informational comment, we could simply immediately resolve it if we didn't feel it needed to be addressed. If however changing the default would be unwelcome, then another possible alternative would be in the settings where you can select "Require conversation resolution before merging", it could offer an additional option, perhaps like: "Also require general PR comment resolution"