RFC: GitHub PR "Resolve Conversation" button

LLVM Project
1

TL;DR: I propose committers of a PR should not use the “Resolve Conversation” button, and that it should be left to the reviewers to use it.

With the move to GitHub PRs basically underway, I wanted to raise the question about how the “Resolve Conversation” button should be used in PRs. This button appears at the end of any comment thread and anybody can click it to hide the thread from view. The thread is listed in the conversation view, but is minimized, so you have to consciously click the button to expand it.

In Phabricator, there is a “Mark as Done” button. This can be used by the committer to mark when they believe they’ve addressed a concern. It is a little checkbox next to each comment. Reviewers can still see the comment, and have the abiltiy then to collapse the conversation if they are happy with it. If, on the other hand, a contributing marks the thing as done, but the reviewer isn’t happy with it, they can still comment on the topic.

In my company’s downstream GitHub Enterprise repo, I encourage the committer to NOT resolve conversations, and then I can use the Resolve Conversation button to mark when I am satisfied with the resolution. If however, a committer were to resolve it, thinking they’ve resolved it, I wouldn’t immediately be able to tell which comments I had yet to confirm have actually been addressed.

For those wondering whether people would ever mark things as resolved when they ever haven’t actually resolved the issue, my answer is yes, regularly (at least in Phabricator) where they mark things as done but never actually address the commented-on issue.

I’d like to propose the LLVM community adopt the same policy that I follow in my downstream project, namely that the committer shouldn’t click the “Resolve Conversation” button, but rather they should leave reviewers to use it.

5 Likes
2

+1

I never know who’s supposed to resolve things on Github. Didn’t really on Phabricator either but since the conversations were still visible it didn’t matter so much.

In practice I probably did a mix, if I thought I had addressed a simple thing I’d just mark it done. Anything more complicated I’d leave it for the reviewer. Which is almost worse now that I think about it :slight_smile:

A clear guide either way is good and I like your proposal.

I’ve been guilty of marking things as done as I do them locally, meaning to push the result when I was done and forgetting to do so.

3

Interestingly, in Phabricator, it doesn’t “submit” the marking of done unless you push an update to the diff, or you explicitly submit them, so that’s a perfectly reasonable thing to do. Plus, the reviewer can reasonably easily tell when the actual code to address something has been pushed, since they get a different notification.

4

I’ve had things marked as Done just because I replied to them, not because they were actually done. One more reason not to like Phabricator.

2 Likes
5

Not sure if this is what you mean, but in Phab, I think the committer’s comments are automatically marked as done (but not reviewer comments).

6

+1 to the RFC, by the way. If the author replies and resolves, the reply isn’t directly visible, and that’s just not good.

7

I often use those done checkboxes as my todo list when going through changes and hit them when I’ve done it. I also don’t really have an expectation that the reviewer will bother to go through and hit done when they’re satisfied (I certainly don’t do that on phabricator)

8

Yeah, I am in the same camp here, in all the different GitHub projects I have been involved in - I have used the resolved thread as a checklist and never seen the reviewer go back and resolve stuff.

We can certainly try to push people to do this, but I think it’s clear that in that case, LLVM will behave differently than other GitHub projects and it might lead to confusion.

I am weak -1 on this RFC for sure.

9

It seems like there’s a tension here between the need for a checklist for committers (to make sure they’ve addressed reviewer comments) and also for reviewers (to make sure their comments have actually been addressed, as opposed to the committer claiming they’ve addressed them). In Phabricator, as a reviewer, I can hide comments that I’m happy have been addressed. Committers can use the “Mark as Done” box to achieve their end.

As someone who almost exclusively reviews code rather than commits code, my view is obviously somewhat biased towards making reviewers lives easier. I also have no idea what the norm is in other projects outside my own team’s one (which involves a team of 2-3 people, so isn’t exactly exposed to a wide range of viewpoints).

10

I’m a +1 here as well. It is important for the reviewer to be able to ensure their concerns were addressed, and having the comment hidden by GH gets in the way of that.

The “checklist” use case for authors is important, but note that if there is a significant amount of time between the item being marked as resolved and the commit being pushed, this creates a very confusing state for the PR. I personally use some uncommon emoji reaction to mark something as done.

11

While I personally love to use these as checkmarks for having addressed review comments, I think it’s more important to optimize reviewer experience, given the shortage of reviewers and the amount of reviews. Therefore +1. I am hoping this will be documented in appropriate places.

12

My idea of optimizing the reviewer experience is to not add additional steps that are expected of me. Most of my comments are one off, I am not coming back to revisit them

13

I think the big majority of the time that will be the author changing their code to a request in review. But occasionally the change author will push back on a review request and if he can convince the reviewer then the reviewer should resolve/press “done” to accept.

14

to not add additional steps that are expected of me.

I agree here as well, but that doesn’t have to mean the PR author should resolve them. IMO it is acceptable for the comment to be left unresolved.

15

So should we also ask the author to comment to mention they addressed something?
(reduce reviewer overload of figure out themselves)

16

I like this idea! But as mentioned, I almost never end up as the committer these days…

Yes, my plan would be to add this to the GitHub workflow documentation somewhere.

Right, that’s exactly what my opinion is. The intent is to leave the option for the reviewer to do it, but not to mandate that they do.

I usually work on the assumption both in my company’s internal repo and when reviewing on Phabricator, that unless the committer has explicitly said otherwise, when they update the review/PR they have addressed everything commented on. I’m not opposed to other suggestions (I like the idea of using emojis for a checklist, for example), but don’t feel the need to propose anything myself.

17

As a patch author, I’m usually listing the changes I made when the patch is updated. But that was on Phabricator. For Github PRs, I’d feel the most natural to break up update into separate commits when possible, which leads to the same list of changes composed of commit titles. With no natural place to list what hasn’t been addressed. Leaving a new comment could be a way for this, though.

Another case is when one of the discussions is blocked on waiting for someone else to chime in, with no clear conclusion. Should the author postpone addressing other comments because of that (or refrain from pushing them)? It’s possible, but feels awkward to me.

I understand (and feel myself) that reviews are bottleneck, but it would be nice if our way of doing reviews wouldn’t end up going against the tide too much, otherwise gains from improved workflow for reviewers might not realize under constant teaching of our workflow, and unaccustomed people making mistakes trying to follow it.

18

+1

In case the author wants to express the comments have been addressed, he can say either “everything except explicit mentioned are addressed” or “everything addressed is replied with done”. I feel it won’t be a big job.

19

Sure, my comment was just a broad brushstroke about how I view things. I think where it’s obvious that a discussion is still ongoing, it doesn’t need specifically stating as such necessarily.

To be clear, my focus of this PR is the specific use of the “Resolve Conversation” button. I think there are related processes that are relevant (committer checklists, how to highlight what hasn’t been addressed etc), but I’m also not proposing any hard and fast rules for those, because I suspect different reviewers and committers will have different things that work best for them. The use of the “Resolve Conversation” button though has the potential to cause serious friction for some people, hence why I think a formal policy around it is needed.

As an interesting point about the norms, we had a new intern start at my company recently and he asked “whose responsibility is it to click the button” when first starting on our local repo, suggesting to me that there isn’t necessarily a universal view on this topic.

20

I’ve been following this thread, and I don’t hold strong opinion regarding your RFC. I see compelling arguments for both ways of placing responsibility to use this button. What I agree with you on is that we should have a policy for this.

That’s wise of him! I agree that there’s no universal view on this topic. I feel that whatever workflow for Github PRs we come up with, we should weight it not just against our community, but against wider community of Github users that reviewers will have to work with as new contributors come in. So I’m just raising awareness here.