pr status: show number of approvals by despreston · Pull Request #2261 · cli/cli

despreston · 2020-10-22T18:53:07Z

This commit adds the total count of PR approvals in the pr status command output.

mislav

Thanks for taking this on!

Let's discuss the design of the feature some more in the original issue thread, but as for the code in question, I've left a comment about correctly parsing Review records.

mislav · 2020-10-26T11:20:07Z

pkg/cmd/pr/status/status.go

+
+				for _, review := range pr.Reviews.Nodes {
+					if review.State == "APPROVED" {
+						totalApprovals++


Note: this will count repeated approvals by the same person as multiple approvals.

I sometimes approve a single PR multiple times, usually to acknowledge that I've scanned over code updates since I've last reviewed. Multiple approvals from the same person should technically count as a single approval.

Moreover, if I leave 2 reviews where the first one is Approved and the second one is Request Changes, then my previous review should be discarded and should not qualify as an approval. Only the (chronologically) latest review status (other than a Comment status, which is neutral) from a single person should ever be taken into account.

See

cli/pkg/cmd/pr/view/view.go

Line 257 in c77e6af

func parseReviewers(pr api.PullRequest) []*reviewerState {

despreston · 2020-11-23T19:26:44Z

@mislav Sorry to bother but do you have a timeline for when this can be reviewed again?

vilmibm · 2021-01-20T18:15:02Z

@mislav

change to N/X

alexcwatt · 2021-04-29T13:52:16Z

In the discussion here it sounded like this could ship as N approvals per @mislav. Or are you looking for N/x approvals @vilmibm?

I'm happy to help with this PR in any way I can. @despreston thanks for working on this 🚀 Having the count of approvals would make my life easier 😄

despreston · 2021-05-03T14:41:42Z

aha is that what the "n/x" means?? I would be happy to update this if that's what's needed, or hand it over to @alexcwatt if he wants to contribute to this.

mislav · 2021-05-07T10:06:56Z

Sorry for the inactivity, y'all.

We decided that if we were to ship this, we wanted to show N/X Approvals, where X is the minimum number of approvals per branch protection rules. Once the approval requirement has been satisfied, we can go back to showing just "Approved" without specific numbers.

alexcwatt · 2021-05-07T11:44:53Z

@mislav Thanks for getting back - that makes sense. In my use case, the minimum hard requirement for the number of approvals of 1 is lower than the soft requirement of 2... so just seeing "Approved" wouldn't help me know which PR's I should still seek a second approval on.

Perhaps there could be some sort of flag to show the counts always?

mislav · 2021-05-07T12:03:30Z

so just seeing "Approved" wouldn't help me know which PR's I should still seek a second approval on.

I see! That's a good counterpoint, thank you.

Let's then go with "N Approved" when the base branch has no minimum approval requirements, and "N/X Approved" otherwise.

From a linguistic perspective, "N Approval(s)" would be more correct, but then we would have think about pluralization rules in the "N/X" case, so let's just not go there right now and stick with "Approved".

alexcwatt · 2021-05-07T12:08:11Z

That sounds great @mislav!

@despreston If you're able to update this that would be awesome. Otherwise I will get to it later (literally getting married tomorrow so not for a little while! 💒).

despreston · 2021-05-07T14:49:58Z

I can take this on. Should have something by next week.

This commit changes the approval count in the PR status command so that it only counts the most recent review from each reviewer.

If the base branch has no minimum approval requirements, show "N Approved" reviews. Otherwise, show "N/X Approved".

despreston · 2021-05-07T19:24:00Z

@mislav Pushed a change but I'm still not 100% I read the api correctly. If that's the case, lmk and I can dig in again.

despreston · 2021-06-04T17:05:54Z

@mislav @vilmibm Can someone please let me know what the status is for this feature? I really dont mind pushing this across the finish line, but it's a bit of a pain to deal with merge conflicts because the PR has been open for so long.

despreston · 2021-08-26T19:54:50Z

lol this is silly as hell but I deleted this branch locally, then changed my mind, and now I can't figure out how to push the changes to this branch. Here are the steps I took:

Checkout des/approval-count from this repo (cli/cli).
Make my changes and commit.
Change the remote to point to my cloned cli/cli repo (despreston/cli).
??? How can I push those changes to this place?

https://github.com/despreston/cli/tree/des/approval-count is the branch with the latest changes. I just needed to fix that latest merge conflict.

alexcwatt · 2021-08-29T17:54:11Z

Change the remote to point to my cloned cli/cli repo (despreston/cli).

??? How can I push those changes to this place?

https://github.com/despreston/cli/tree/des/approval-count is the branch with the latest changes. I just needed to fix that latest merge conflict.

git push takes an argument for the remote. I'm not sure if you're saying that you can now do git push origin and push to your remote, or if you need to use a different name. https://git-scm.com/docs/git-push

mislav · 2021-08-30T14:17:03Z

@despreston You have deleted the head branch; therefore this PR had stopped receiving updates from your fork. It doesn't matter that you later pushed the branch again—I don't think the PR can be "fixed" again. Please open a new PR!

despreston · 2021-08-30T14:22:18Z

Lesson learned: don't delete the head branch before the thing is merged.

mislav suggested changes Oct 26, 2020

View reviewed changes

despreston requested a review from mislav October 29, 2020 14:22

vilmibm assigned mislav Jan 20, 2021

pr status: show number of approvals

0d3e580

despreston added 2 commits May 7, 2021 11:05

pr status: fix approval count

68e4045

This commit changes the approval count in the PR status command so that it only counts the most recent review from each reviewer.

pr status: show required approvals

7e8ea95

If the base branch has no minimum approval requirements, show "N Approved" reviews. Otherwise, show "N/X Approved".

mislav added the enhancement a request to improve CLI label Aug 4, 2021

despreston closed this Aug 30, 2021

mislav mentioned this pull request Aug 30, 2021

pr status: show number of approvals #4240

Merged

Conversation

despreston commented Oct 22, 2020

Uh oh!

mislav left a comment

Choose a reason for hiding this comment

Uh oh!

mislav Oct 26, 2020

Choose a reason for hiding this comment

Uh oh!

despreston commented Nov 23, 2020

Uh oh!

vilmibm commented Jan 20, 2021

Uh oh!

alexcwatt commented Apr 29, 2021

Uh oh!

despreston commented May 3, 2021

Uh oh!

mislav commented May 7, 2021

Uh oh!

alexcwatt commented May 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mislav commented May 7, 2021

Uh oh!

alexcwatt commented May 7, 2021

Uh oh!

despreston commented May 7, 2021

Uh oh!

despreston commented May 7, 2021

Uh oh!

despreston commented Jun 4, 2021

Uh oh!

despreston commented Aug 26, 2021

Uh oh!

alexcwatt commented Aug 29, 2021

Uh oh!

mislav commented Aug 30, 2021

Uh oh!

despreston commented Aug 30, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

alexcwatt commented May 7, 2021 •

edited

Loading