GitHub: When to mark a PR "conversation" as "resolved"?

[webbnh]

Assuming that we yet have a number of pull request code reviews that we're going to conduct on GitHub, I think it might be handy if we shared some conventions around who marks a PR "conversation" as resolved and when.

I think its a good goal to have all conversations marked as resolved before a PR is accepted and merged. I'm not sure it's feasible to make this a hard requirement, but I think it's a worthy goal. Also, I think that, with some care, the open-vs.-resolved state can provide some nice support for us, indicating to the author of the PR (and the admin) when there are outstanding items or when there are issues which are under contention. The problem is, there is only one button, and it's not clear who is supposed to push it and what the conditions are which need to be met before it can be pushed. (Let's set aside for the moment that GitHub's presentation isn't the most graceful, and that it frequently hides conversations, even unresolved ones!)

A conversation starts with a comment on a code review from someone that we'll call the "reviewer". (For the moment, let's assume that it doesn't matter whether this person is an assigned reviewer or just a passer-by.) The reviewer might be making a comment, requesting a change, or asking a question. (Of course, just because it looks like a comment or a question doesn't mean it's not a request for change, and just because it seems to be a request for a change doesn't mean that it's not just a comment or possibly even a question, but I think the root of the communication falls into one of those three categories.)

If it's a comment, then the reviewer probably doesn't need anything more than acknowledgement that the author has read it and given it due consideration. In this case, having the author press the "resolve conversation" is probably sufficient. However, if the author feels inclined to respond to the comment, then we should probably treat it more like a question.

If it's a question, then the reviewer probably wants to see the author's response. This suggests that the author should not be the one to resolve the conversation: once the conversation is marked resolved, it is likely to be hidden at least one and possibly more clicks away from the reviewer's sight. Instead, it should probably be up to the reviewer to mark it resolved, assuming s/he is satisfied with the author's response. If the reviewer is impelled to post a followup, then the situation reverts to the initial state and we start only.

If the conversation begins with or reaches a request for a change, then things get murkier. Presumably, the author (if s/he agrees with the request) will make the change and request a new review from the reviewer. In the meantime, the open conversation serves as a reminder to the author of the outstanding issue. Once the requested change has been made and the reviewer is satisfied with the change, then the conversation can be resolved. Until then, the open conversation serves as a reminder to the reviewer to check for the change in the revised PR. So, here's the rub: until the author has made the change, both parties want the conversation open; once the author makes the change (long before the update is reviewed), the author wants it closed; however, until the reviewer does his/her re-review, s/he is best served by keeping it open.

So, to distill, it seems like the person who should mark a conversation resolved is the one who didn't "answer" or otherwise respond. That is, if the last post was a comment or answer, then the recipient (who doesn't otherwise respond) should mark the conversation as resolved (as an acknowledgement of the last post and in lieu of any other response). This works for conversations which start (or continue) as questions, with the questioner accepting the other party's answer by resolving the conversation. In the case of a requested change, the change itself is the answer or response, and the requestor should close the conversation when the change is accepted.

I think this covers things fairly well, but it does leaves us with two problems. First, the unresolved conversation list (if only it were actually available as a list!...) doesn't serve the author very well in terms of representing outstanding issues. However, perhaps this can be addressed by having the author make followup comments or by using emotes. These aren't as powerful as resolving the conversations, because they don't cause the conversations to be hidden from the author's view, but it's something, at least. Second, and more importantly, it doesn't support conversations with more than two people in them. However, these are uncommon, and in most cases it's obvious who the Reviewer is and who is just a Kibitzer, and the latter will just have to take his lumps -- if necessary, s/he can open a separate conversation to track his/her own facet of the issue and thereby become a Reviewer.

This was mostly just a brain dump -- it's not necessarily meant to be a concrete proposal. What does everyone else think??

[dbutenhof]

That's a good use for Discussions... and I've had similar thoughts myself over time but never paused to write them down, so thanks for pushing the conversation.

I think the Gerrit model works a lot better here, but there's still need for some conventions and I've never managed to work out any clear protocol that's easy to follow and makes everyone happy.

In general I agree with your suggested model, though I have a feeling it's not going to be quite so straightforward in reality.

And at the heart is the fact that GitHub often makes "discussions" extremely hard to find, especially after many reviews, and through making files "Viewed" which I've found fairly important in managing the complexity of repeated reviews.

A view that showed all the unresolved comments would be an enormous help here; without it I fear we're going to be thrashing in the dark more often than not.

[webbnh]

That's a good use for Discussions...

Peter made me do it. ;-)

With regard to finding unresolved conversations, the best approach I've found is to go to the PR "home page" where it shows the chronological event list, and look for the conversations there. In order to be effective, you have to find all the places where GitHub "helpfully" hides them for you (look for "hidden items") and re-expand them. Looking for them in the file diffs often doesn't work, especially if the conversations referred to superseded commits or to code lines which have since been removed.

[dbutenhof]

Yes, of course the comments are there and can be exhumed; but it's supposed to be a code review, not a treasure hunt, and I continually gain the sense that GitHub has its own agenda in mind and doesn't care much about ours...

[portante]

I think its a good goal to have all conversations marked as resolved before a PR is accepted and merged.

+1, that is the model I use with all of my PRs.

Not to dismiss much of what you wrote, but fundamentally GitHub conversations is a very flawed model. They are attached to versions of code which are dropped by commits are amended, and they are displayed in some sequential fashion, with no facility to query which ones are resolved or unresolved, and whole groups of conversations will not be displayed.

The Gerrit method of having comments attached to patch sets is fundamentally the better method, since the patch set is never removed, and the comments can be reviewed by patch set for a given commit.

That said, given the current model, I think the owner of the PR should be responsible for resolving the conversation. If we agree that during a review we don't amend commits, then the conversations will be available off the commits, and if a reviewer does not agree with a conversation being closed, they can re-open it.

[dbutenhof]

That said, given the current model, I think the owner of the PR should be responsible for resolving the conversation. If we agree that during a review we don't amend commits, then the conversations will be available off the commits, and if a reviewer does not agree with a conversation being closed, they can re-open it.

So, more algorithmically, you're saying

1. Reviewer posts a comment,
2. Submitter makes a change, or (in their view) answers a question, and marks the conversation "resolved",
3. Reviewer finds their resolved comment, and either (a) accepts the answer and leaves it resolved or (b) disagrees and "unresolves" the answer, adding another link to the chain

Do I have that wrong??

One problem, or at least inconvenience, with that seems to be the process of going through all of the closed/resolved conversations to find the right one; GitHub doesn't make this easy since a resolved/closed conversation doesn't even show a summary.

Leaving the conversation unresolved until the reviewer accepts the answer makes it all just a little bit more visible:

1. Reviewer posts a comment,
2. Submitter makes a change, or (in their view) answers a question,
3. Reviewer finds their open comment, and either (a) accepts the answer and marks it resolved or (b) disagrees and adds another link to the chain to ask a clarifying question or to argue their intended change; rinse and repeat.

This treats the reviewer as always the "owner" of the comment, which I think makes sense, and the ultimate arbiter of when the concern is resolved or the question is answered. (Not to mention of how much better off we'd all be if we could get off GitHub to that mythical "something better".)

[portante]

The problem is tied to commit amendments.

If you amend a commit, and that commit changes a file which has a comment on it, it gets "lost" easily.

If we don't amend commits, then you can go back to previous commits to see your comments that have been resolved.

[dbutenhof]

You can go back; but you have to open each resolved comment to find out what's in it, which is a pain. The GitHub review workflow just sucks massively, and while the pain can be slightly minimized by convention (ice packs and ibuprofen, mostly), it's always going to hurt...

[webbnh]

My concern with having the PR owner mark the conversations as resolved is that it increases the burden on me as a reviewer. If I have flagged an issue, then it should be incumbent upon me to check that the issue has been resolved prior to giving giving my approval; therefore, it's my responsibility to maintain a list of items that I've flagged and to mark them off when they have been addressed. It would be great if GitHub would do that for me, e.g., by allowing me to comment on code lines, display my previous comments, and then allow me to mark the ones which have been addressed as complete...which, it does...but only, if I as reviewer have the responsibility for pressing the "Resolve conversation" button. If the PR owner marks the conversation as resolved, then I either have to check all the resolved conversations manually, or I have to maintain an external list...neither of which is a good solution.

Now, I acknowledge that the PR owner faces an analogous problem. S/he needs to be able to go down the list of flagged items and mark the ones which have been fixed in some way so that s/he knows when s/he is "done". But, marking the conversations as resolved doesn't work well for this, because resolution is in the eye of the reviewer (i.e., the one who is or isn't ultimately going to approve the overall PR). The fact that the reviewer can reopen the conversation is not a solution to this dilemma -- in fact, it makes the problem worse: it allows us to enter a loop of the reviewer reopening conversations which the owner closes, which creates confusion as, at any particular moment, we cannot tell which issues are open and which are actually resolved.

I would suggest instead that the PR owner can use an emoji to mark the conversation as "fixed" -- there are eight of them -- we should be able to assign conventions or actual semantics to their use, reserving one for the PR owner. (Or, the PR owner could just reply to the conversation indicating that it's been addressed, but this generates e-mail which the emojis do not.)

[As I indicated below, problems with conversations "getting lost" can be addressed straightforwardly -- and GitHub is improving things -- and, frankly, it's not hard for a reviewer (who has to locate them all, anyway) to point out any which have been overlooked by including links in subsequent comments.]

[webbnh]

Good news, everybody! GitHub now offers a widget for getting a list of all of the unresolved (and all other) conversations! Unfortunately, it doesn't help you locate them except in fairly specific circumstances, but you can at least view the list.

In any case, the conversations, even if they are attached to code lines which were later deleted or to commits which have since been amended, are always available on the main PR page (under the "Conversations" tab). You just have to scroll up, and possibly expand the sequence when GitHub elides it for you. If we are diligent about resolving conversations, then the unresolved ones are not hard to find.

[dbutenhof]

When you're looking at the Files Changed tab, there's a "Conversations" pulldown that'll list all open conversations on the PR:

It's not perfect, especially when commits are rebased, and often won't navigate directly to conversations in files with a lot of changes; but, at worst, it's a list of all the places you need to find on your own. 😆

[OliverPearmain]

@dbutenhof 🙇 much appreciated

[webbnh]

@OliverPearmain, there's also a link at the bottom of the main ("Conversations") tab of the PR page, in the status "block" which floats at the end of the conversation list. There is a summary count of the open conversations, and at the right side of that area there is a hyperlink, "view", which will also take you to the list which Dave pointed out.

There are a couple of things to note about GitHub "conversations". First, the are displayed in two places: they can be found on the page under the "Conversations" tab of the main PR page, and they can also be found under the "Files changed" tab (with some caveats...read on). In the Conversations tab, the conversations are listed in time order; if the conversations are "pending" (which only their author can see), then they are entered by their creation time; once they are submitted, they appear in order of their submission time. "Conversations" (as opposed to "comments") are grouped under the review which they are part of. (Note that GitHub tries to update this page dynamically, but it doesn't always succeed entirely...take care if you happen to have the "Conversations" tab open in one window and one or more "Files changed" tabs open in other windows, because the state of your review is maintained on the server side.)

In the "Files changed" view, conversations are attached to specific code lines in specific revisions of specific files. What this means is that, if you take advantage of GitHub's capability of displaying only certain ranges of commits within a branch, conversations which aren't attached to those revisions of those files may not be displayed in the diffs. Also, if a subsequent commit changes the specific lines to which a conversation is attached, the conversation will be marked "outdated" and will not be displayed. Likewise, if the commit itself to which the conversation is attached is removed from the branch (i.e., through a git commit --amend or a merge or rebase), then the conversation will become "outdated" as well. (See below for the implications of this.)

Each conversation has a unique URL which allows them to be hyperlinked-to, which can be very handy. (You can find this as the target of the date in the header of the conversation.) However, note that a conversation in the "Conversations" tab has a different URL from the corresponding one in the "Files changed" tab, even though they are the same conversation (i.e., a reply to it will appear in both places). However, just because they can be referenced doesn't necessarily mean that you can just load them. GitHub apparently has challenges serving large PRs, and as a result they engage certain optimizations. For instance, in the "Conversations" tab, once the page exceeds a certain length, GitHub will "hide" conversations in the middle of the stream, replacing them with a box indicating how many are hidden and providing a link which will cause them to be loaded. If a URL links to a conversation which has been hidden, GitHub (or, perhaps more accurately, the browser) may not be able to reach the conversation. Likewise, in the "Files changed" tab, diff's over a certain size aren't loaded initially; and, because they aren't loaded, any conversations attached to them are not loaded...so, they cannot be referenced directly by their URLs, either. (And, of course, conversations attached to commits which have been amended cannot be reached in the "Files changed" tab, ever.)

Nevertheless, the "Conversations" drop-down menu, does manage to keep track of all the open conversations, even if they are outdated. However, the entries in that menu use URLs which refer to the "Files changed" display, which means that, if they are outdated, clicking on them doesn't work. Nevertheless, what you can do to find them is to look in the drop-down and get the date of the conversation, and then you can switch to the "Conversations" tab and find the appropriate point in the timeline to locate the conversation.

[dbutenhof]

And if all of that sounds confusing and perhaps somewhat "intellectually unsatisfying"... it's largely because of Github "quirks" like this that we have been considering the logistics of moving code reviews to Gerrit, which has (in my opinion, shared by some others) a much more rational model. 😆

[webbnh]

Wow. I just can't wait to hear how we are going to get to Gerrit...so that we can stop bringing it up every time we talk about how to work with GitHub. 😭

[ArchitectAK]

Personally, I like to leave it up to the reviewer to resolve the conversation.
The way I see it,

if a reviewer commented on my PR,
it's up to them to also verify that the comment has been addressed,
and mark the discussion as resolved if it was.

[OliverPearmain]

Personally, I like to leave it up to the reviewer to resolve the conversation.

If only we could enforce this

[webbnh]

We are counting on people being responsive (or at least responsible...). If it becomes a problem, we can address it "manually" in individual cases.

[portante]

This (#2113) is also discussed in #2555 and #2339. The fact that we have 3 discussions on this topic, and it is still a sore point with some means we need to deal with this going forward.

And reading through this I don't believe the tool helps us deal with unresolved conversations. Too many times I have to track down a reviewer who just leaves a comment but approved the PR and the PR languishes until it is resolved.

[webbnh]

This (#2113) is also discussed in #2555 and #2339. The fact that we have 3 discussions on this topic, and it is still a sore point with some means we need to deal with this going forward.

Actually, it's not -- the three Discussions are addressing distinct topics:

• This discussion deals with the responsibility for marking a "conversation" as "resolved".
• Let's reconsider approvals vs unresolved comments #2555 discusses the reviewers' choices of "Approved", "Comment", and "Changes Requested".
• GitHub PR process #2339 discusses techniques for making GitHub serve us better.

Trying to tie them all together will not help our situation.

[MarqueIV]

The crux of the problem, at least for me and seemingly others, is 'Resolved' doesn't cover the actual use-case that such comments are trying to achieve. Instead of focusing on the tool, focus on the task. What are you trying to do with a comment requesting a change or even just asking for clarification? You're trying to signal to the author, 'Hey... can you address my concern here?'

And that's the difference. An author is addressing the concern, or trying to, but it's the reviewer who can say if that does resolve the issue.

But simply commenting back to the reviewer doesn't give them any signal where they can quickly say 'Show me all my issues that have been addressed' because a comment from the author has no state!

That's why for me, what's missing is that interim step... 'Addressed'. It's something that lets an author signal back to the reviewer 'Hey... I believe I have addressed your concerns so get your eyes back on this.' This lets the reviewer quickly grep what the authors have or haven't (yet) addressed, and determine of those that have been if they're addressed satisfactorily, or to put another way... if they're 'Resolved!'

If it's something simple like fixing a typo, then an author can and should be able to resolve them as that's not something a reviewer would need to 're-review', but for all other cases, they should exclusively use 'Addressed.'

So to summarize, new comments from a reviewer puts the comment thread in the 'Unaddresssed' state. An author can respond themselves and/or move it to the 'Addressed' state. From there, the reviewer can quickly see what's addressed, and if appropriate, move them to the 'Resolved' state. If they add another comment, it goes back to 'Unaddressed'.

That's my take anyway. Focuses on the task, not the tools.

[dbutenhof]

Summary: GitHub is not an ideal tool for code review?

Yes, I'd love to be able to have a developer explicitly mark a conversation as "addressed" (whether by explicit reply or by a checkbox) so that a reviewer can do a query for "addressed" conversations to decide whether to "resolve", and a developer can do a query for "unaddressed" conversations to decide how to address them. (And, ideally, this would come with a view where the diff between the time of the comment and "now" makes it easy to see what, if anything, changed.)

Which all sounds like a GitHub UI suggestion should someone have time and inclination to pursue that course. 😆

[webbnh]

@MarqueIV, I don't really see the need for an "Addressed" state. To me, there are two ways for the Author to respond to a comment: either make a code change or reply in the conversation. The later is concrete and obvious to the Reviewer (s/he gets a notification for it); the former should be reasonably obvious as well and presumably should be the cause for another review pass (and the Reviewer gets a notification for it, too). (Either way, it's up to the Reviewer to determine whether the Author's response (whichever mode) resolves the conversation.)

I don't think the Reviewer needs a "show me all my issues that have been addressed" feature. What is needed is a lower-friction way for the Reviewer to find all of his/her unresolved conversations, so that s/he can mark as resolved the ones which have been successfully addressed. Optimally, the Author should address all of the unresolved conversations before the Reviewer makes his/her next pass, so there shouldn't be much need to discriminate between "addressed" and "unaddressed" conversations (since, at that point, they all should have been addressed).

It would be nice if there were also a lower-friction way for the Author to get access to all the unresolved conversations, although there is the "Conversations" pull-down on the "Files" page, and the "Conversations" page isn't terrible (but GitHub has a habit of hiding unresolved conversations when the review gets big), but these end up being cumbersome in various ways.

If it's something simple like fixing a typo, that is trivial to review, so it really doesn't save much to lift that responsibility off the Reviewer; on the other hand, it is often the case that addressing typos turns into renaming or rewording things, and then the Author really should not be resolving the conversation without the Reviewer's input. So, it's just clearer and simpler to state that in all cases the person who started the conversation should be the person to mark it resolved -- that produces a closed loop.

Focuses on the task, not the tools.

But, it sounds like you're trying to "fix" the tool. 🙂

I'd love to be able to have a developer explicitly mark a conversation as "addressed"

Yeah, but...I can't count the number of times I've had the Author reply to me as Reviewer and say some variation of "Fixed!" only to find that the fix didn't make it into the branch. So, being able to jump to "addressed" comments doesn't do much for me, especially since I'm going to review all the changes in the commits which were added after my last review anyway. So, I'll get to see the fixes, regardless of whether the Author marks/acknowledges them or not.

The pain point for me as a Reviewer is when an Author misses outstanding open conversations...despite the existing mechanisms which GitHub provides (you can scan the "Conversations" page; at the bottom, it includes a count of open conversations, and in this repo it puts a big red circle next to it if there are any open ones; and, on the "Files" page, there is the pull-down which gives you counts and links to them). That's just frustrating for all parties involved, especially the ones who don't understand why the review process takes so long.... 🙃

And, yeah, it would be cool if you could click on a conversation and say "show me what has changed since I opened this", but I'm afraid that that is a hard problem to solve automatically.

[danielhouriez]

When someone makes a change as a result of a comment, they may have addressed the problem but not necessarily resolved it, e.g., they may have misunderstood the ask or missed something. It should be up to the person who posted the comment or any senior developer / admin to mark it as resolved.

I think adding a reaction on a comment to indicate it has been addressed would be the way to go. In Bitbucket, one may add a "checkbox" reaction on the comment but GitHub reactions are very limited. There's a "thumb up" but that does not have the same meaning. A feature request in GitHub for custom reactions is open but i wouldn't hold my breath...