PR template update: Ask that contributors not resolve conversations themselves

Django Internals
1

Hey folks,

One common thing I see in PRs is authors marking conversations resolved by themselves. I’ve always asked teammates to leave this to the review/merger to do because it’s not up to the author to decide whether a reviewer’s comment has been addressed or not. Additionally when authors do this it literally hides the conversation from other reviewers/mergers (by default until expanded). I find this to be counter-productive.

Can/should we add a note to the new PR template to ask folks to not do this?

2

I must admit that I resolve conversations on my own PRs to help me track what I have done and what I haven’t done. Resolving very small suggestions for things like typos I think is fine? (this is where I learn everyone hates it :joy:)

I guess the issue is if people are resolving non-trivial discussions or suggestions which the reviewer/merger needs to validate was resolved properly.

GitHub also starts to hide conversations and items on it’s own when there are a lot (like in this PR: Accessibility guidelines for all contributors by thibaudcolas · Pull Request #17338 · django/django (github.com)). It also marks some things as ‘outdated’ when the comment was against code that has changed and this can also hide comments.
So unfortunately, I think if things being hidden is a bad thing, I don’t think GitHub is helping.

The more complicated PRs receive so much feedback and keeping on top of all of it is tough.
Maybe it’s worth us organising the sprint retrospective idea, so we can gather feedback on the pain points of the review process.

Note: if I see that you didn’t resolve the conversation yourself and it’s non-trivial, I could confirm with you if you think it’s addressed? We could maybe have that you emoji react :white_check_mark: when they resolved it but you’re happy it’s resolved.

1 Like
3

In my experience, Django contributors can often be trusted with resolving conversations. When I’m writing a PR, I also like being able to hide a thread, especially when it’s something trivial.

I’d also prefer that we don’t bloat the PR template with lots of rules, as that will probably lead to none of them being read or followed.

1 Like
4

I’ve always assumed this was the right and expected procedure :scream:

I would suggest that we don’t this, for the following reasons¨

  • In my experience, when conducting a review I would expand all comments to read and get familiar with past conversations, both to be aware of what was discussed and agreed so far, and to ensure that agreements/resolution follows the desired/documented patterns. This also allows for double checking that a comment was addressed as expected.
  • Also, once a conversation is resolved, GitHub does not show who resolved it. So as a safety net we (reviewers/mergers) would necessarily expand all/most of the comments and review its outcome to confirm it was properly resolved. Unless there is a setting in GH that would help in this front?
  • Lastly, as Sarah says, resolving comments in our own PRs allows for an author to have a better track of what’s pending and what was addressed. IMHO this also encourages having a sense of ownership of the PR/fixes/comments.

Thank you!
Natalia.

1 Like