Skip to content

Implement review roulette - #1270

Closed
kennethshsu wants to merge 1 commit into
mainfrom
#1239_PR_review_roulette
Closed

Implement review roulette#1270
kennethshsu wants to merge 1 commit into
mainfrom
#1239_PR_review_roulette

Conversation

@kennethshsu

@kennethshsu kennethshsu commented Sep 2, 2026

Copy link
Copy Markdown
Member

Summary of Changes

This should setup the roulette properly. Reviewer will be randomly selected from chainladder-maintainers, using the load balance method (if you already have a lot of pending work, you'll less likely to be selected).

For anyone interested in becoming a reviewer, they'll just need to be added as a member to chainladder-maintainers. Still only chainladder-codeowners can merge to a protected branch.

Related GitHub Issue(s)

Closes #1239

Additional Context for Reviewers

The only "code change" here is to the file CODEOWNERS, which is the random selection pool of PR roulette, and it should point to chainladder-maintainers.

Checklist

  • I passed tests locally for both code (uv run pytest) and documentation changes (uv run --directory docs jb build . --builder=custom --custom-builder=doctest)

Note

Low Risk
Only updates GitHub review routing in CODEOWNERS; no application or library code changes.

Overview
Review roulette for this repo now draws reviewers from the @casact/chainladder-maintainers GitHub team instead of four named individuals.

The only change is .github/CODEOWNERS: the wildcard owner line is updated so automated review assignment (with load balancing across pending reviews) uses the team pool described in the PR.

Reviewed by Cursor Bugbot for commit f5d7516. Bugbot is set up for automated code reviews on this repo. Configure here.

@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown

Pyright Type Completeness

View the full pyright --verifytypes output for this commit

Project (full chainladder package, at this PR's head): 15.2% of exported symbols fully typed (209 / 1377)

Known Ambiguous Unknown Total
Project (head) 209 111 1057 1377

Other symbols referenced but not exported by chainladder: 13

Known Ambiguous Unknown Total
Other (head) 3 1 9 13

Symbols without documentation:

  • Functions without docstring: 325
  • Functions without default param: 0
  • Classes without docstring: 10

Patch (exported symbols added or changed by this PR): no exported symbol type-completeness changes detected.

@codecov

codecov Bot commented Sep 2, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.70%. Comparing base (e03d708) to head (f5d7516).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1270   +/-   ##
=======================================
  Coverage   91.70%   91.70%           
=======================================
  Files          93       93           
  Lines        5434     5434           
  Branches      700      700           
=======================================
  Hits         4983     4983           
  Misses        327      327           
  Partials      124      124           
Flag Coverage Δ
unittests 91.70% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@henrydingliu

Copy link
Copy Markdown
Member

i'm not in favor. there's not enough review bandwidth as is. roulette solves a non-existent problem.

@kennethshsu

Copy link
Copy Markdown
Member Author

Sorry, just to clarify, are you not in favor of the method of implementation or are you saying we should not do review roulette? Or having the issue with load balancing?

We all agreed that a reviewer should be explicitly assigned for accountability sake instead of automatically assigning PR to all codeowners right? This is in the governing doc, though I think the wording can be improved. We can discuss more at our internal call with @genedan.

@genedan

genedan commented Sep 3, 2026

Copy link
Copy Markdown
Member

I think if PRs are assigned to 1 person, there's a risk that the review will be delayed (even for a regular contributor such as myself). Sometimes we have things like vacations or busy times at work which can bog 1 person down for a week or longer. And sometimes we're just on a break.

I also would expect some of the new reviewers we plan to recruit might not contribute regularly enough so that we have a smooth review cadence at the individual level - they might have "bursts" of activity such as making contributions for 3 weeks in a row and then being absent for another 3, then come back etc.

If we can recruit enough reviewers who take on PRs bottom up (oldest to newest), we can construct a review machine that closes them out quickly on aggregate despite some individual reviewers being out of pocket at any given time.

@kennethshsu

Copy link
Copy Markdown
Member Author

Ok I hear you both. Clearly reviewing is a bottleneck for us, I'll organize some of my thoughts and we can discuss more at our next call.

@kennethshsu kennethshsu closed this Sep 3, 2026
@kennethshsu

Copy link
Copy Markdown
Member Author

Let be close this PR for now.

I don't think any of the back end changes impacts anything.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Set up PR roulette

3 participants