Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add a condensed list of reviewers and status on pulls list #2495

Closed
ashanbrown opened this issue Oct 17, 2019 · 4 comments
Closed

add a condensed list of reviewers and status on pulls list #2495

ashanbrown opened this issue Oct 17, 2019 · 4 comments

Comments

@ashanbrown
Copy link

Consider this example at https://github.com/sindresorhus/refined-github/pulls:

image

If we visit the PR, we see:

image

Perhaps instead of saying "Changes Requested" we could show the reviewer name and the a symbol (approved, requested changes, awaiting review) for each reviewer.

Stepping back to the use case, I visit the pulls page to find out the status of PRs I have created. I have a browser bookmark of pulls created by me. What I'm typically looking for when I visit this page is information that will help me move my PR towards the merged state. Typically this means I have to ask myself "do I need to add another reviewer?" or "do I need to nag an existing reviewer?". A list of pending reviewers and their states would help me answer these questions. I'm not sure if the same could be said for the list of assignees. I don't use that feature but I imagine it has the same concern. Thanks for your consideration.

Example URL:
https://github.com/sindresorhus/refined-github/pulls

@fregante
Copy link
Member

I thought about this for a while and I came up with this: the last-updated date is more important than any of this information. You don’t want to bother anyone too often, right?

I think a PR would be either unread or updated a long time ago to make you open it again. Why open it otherwise? If the last time you read it was 3 hours ago, you probably don’t need to bug the reviewer yet.

And when you do finally open it in one of those 2 cases, you can decide whether to bug anyone.

@zadjii-msft
Copy link

If I could play devil's advocate for a moment: in my workflow, knowing how many people and exactly who's signed off actually does feel valuable. For context, I work on the Windows Terminal, and this year we've moved all our development from Azure DevOps to github. One of the features I really liked aboud ADO was that each PR would show me who on the team has signed off or requested changes, etc:

image

This is important to me, because for our PRs, we need to have at least two members of the team sign off, and have no one still requesting changes. It'd be really helpful to know at a glance what the state of my PRs was in:

image

For example, microsoft/terminal#3369 in the above screenshot actually has two sign-offs, but one person's still requested changes. I'd love to be able to know who that is, without opening the PR, so I can go message them and bug them to take another look.

Or, when looking through the big list of PRs, if there are already two team members engaged on an external contribution, oftentimes that's good enough, and I won't need to also hop on that thread. If I could quickly scan those PRs to see who needs some extra reviewers, that would be valuable to me.

@fregante
Copy link
Member

fregante commented Nov 11, 2019

at least two members of the team sign off, and have no one still requesting changes

All of that is enforceable by GitHub. The second part is already visible in the list: If it's red, someone is still requesting changes.

If required reviews are enabled and a collaborator with write, admin, or owner access to the repository submits a review requesting changes, the pull request cannot be merged until the same collaborator submits another review approving the changes in the pull request.

From https://help.github.com/en/github/collaborating-with-issues-and-pull-requests/approving-a-pull-request-with-required-reviews

What sounds like you need is to show whether a PR is "blocked" by said condition.

And... it's there too. Open https://github.com/microsoft/terminal/pulls and point at the "Review required" items. Some of them already have 1 approval but still need more.

if there are already two team members engaged on an external contribution, oftentimes that's good enough

Now that's finally something that's missing: participating users count.

Feel free to suggest that counter in a new issue.

I'm hesitant to add avatars because PR lists are already pretty busy.

@zadjii-msft
Copy link

Oh certainly, we're already using all those github rules for the repo. However, I feel like mentally, there's a difference between:

  • one person having signed off and another blocking the PR
  • two people having blocked the PR
  • one person blocking and no other reviews

And all those scenarios are just lumped into a singular "Changes Requested" tag.

Participating user count might be helpful enough. I just like being able to skim the PRs list and know who's been active and who hasn't.

If I were to throw together a PR, would this be acceptable as an off-by-default feature? Having it be opt-in would certainly help mitigate concerns about busy-ness of the PR list. I'm not sure exactly what the contribution flow is here, though I'd be happy to learn 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

3 participants