Count all review states toward the PRs Reviewed metric - #516
Conversation
"PRs Reviewed" only counted PRs where you submitted an APPROVED review, so comment-only reviews and change-requests were invisible — arguably the reviews that take the most effort. A thorough review that sends a PR back, where the author then merges after addressing the feedback without a re-review, counted as zero. Reviews now count in any submitted state (APPROVED, CHANGES_REQUESTED, COMMENTED, DISMISSED). PENDING stays excluded: it is an unsubmitted draft only its author can see. The reviewer escape rate keeps the stricter approval-only denominator via the new approvals_only flag — that rate is "regressions I approved / PRs I approved", and you can only let a regression through on a PR you approved. No additional API cost: the reviewed-by: search already returns exactly the PRs the user reviewed in any state; we just read the state field we were already paginating over. Measured effect on one engineer (bkraeling): 7d 17 -> 20 (+3) 30d 61 -> 71 (+10) 90d 139 -> 162 (+23) Co-Authored-By: Claude Opus 4.8 <[email protected]>
|
@redreceipt Thoughts on this? Totally your call here, but this covers a gap where someone does an initial review but doesn't do the final approving review not actually counting as a review in their stats. |
|
Yeah, that shouldn't count. There's no risk or burden in just posting a bunch of comment reviews everywhere and shouldn't be incentivized. |
|
But we're incentivizing review approvals because those count toward stats over non-approvals, right? Comment reviews are basically rendered useless; if I review a PR but have clarifying questions, I now have to request changes on the PR to make sure that the PR gets held up on my review so that I can get credit for the review I did. If I review a PR but think it could use another set of eyes on it, instead of commenting on the review, I now have to approve it, meaning that the person it's waiting on could potentially miss out and the PR gets merged before they see it. None of these situations make our code better, or merge faster. |
|
it's easy to comment on all PRs, everyones bot is already doing that. it's hard to see it through. that's what I want to measure. |
|
Often times I will leave a COMMENT review because i'm not sure if something is merge blocking or not and don't want to falsely approve, or falsely condemn a PR. |
|
Can we count rejections as reviews? I agree that comments should not count - we should incentive folks to move the ball forward w/ a review, and a comment rarely does that. |
|
willing to discuss it but I don't really want to incentivize rejections. everyone already knows they have 24 hours to say something, rejections are just the better versions of comments to me and there's not a a lot of risk or burden involved in saying "I want to think about this longer" |
|
Can we tweak and reopen this PR? For |
|
I'm changing the language to PRs Approved as a start, that what I want to measure for now |
Opening as a draft for discussion — this changes a leaderboard/person metric, so I'd rather we agree on the definition before it lands.
Issue
"PRs Reviewed" only counts a PR if you submitted an APPROVED review on it. Comment-only reviews and change-requests count for nothing.
That has a perverse effect: the reviews that take the most work are the ones that don't count. If you review a PR thoroughly, request changes, and the author addresses the feedback and merges without asking you for a re-review, your contribution is invisible to the metric. Conversely a one-click approval counts fully.
The mechanism is in
get_merged_pr_counts_for_user: thereviewed-by:<user>search matches PRs you reviewed in any state, but we then filter each PR's reviews tostates: [APPROVED]and only count it if you authored one.Solution
Count a PR as reviewed when the user submitted any review on it:
APPROVED,CHANGES_REQUESTED,COMMENTED, orDISMISSED.PENDINGstays excluded — it's an unsubmitted draft only its author can see.Two things I deliberately kept intact:
approvals_only=Trueflag thatregressions.pypasses. That rate is "regressions I approved / PRs I approved" — you can only let a regression through on a PR you actually approved, so broadening that denominator would understate the rate. This is the main thing I'd like a second opinion on.reviewed-by:search already returns exactly the PRs the user reviewed in any state, and we were already paginating those nodes; we just read thestatefield too. Same request count, same pagination.Measured effect
On one engineer (
bkraeling), old vs new count:Roughly a 16–18% undercount for that reviewer. Underlying review states in the 90d window: 139 approved, 50 commented, 18 changes-requested.
Worth discussing
DISMISSED: I count it (the review work happened). Easy to drop if the team disagrees.jobs.py) use the same function, so daily numbers move too.To Test
pr_countdenominator are unchanged by this PR (the regressions path passesapprovals_only=True)python -c "import github; print(github.get_merged_pr_counts_for_user('<user>', 30), github.get_merged_pr_counts_for_user('<user>', 30, approvals_only=True))"— second value matches the pre-change numberruff check .,ruff format --check .,mypy .,vulture . --config pyproject.toml, unit testsVerified locally: all four CI gates pass and the suite is green (195 tests, including one new case for
approvals_onlyand an updated case for the broadened counting).🤖 Generated with Claude Code