Skip to content

Conversation

@sethrj
Copy link
Member

@sethrj sethrj commented Dec 5, 2025

#2113 has led to too many people being requested by default as codeowners. This reduces the numbers of reviewers requested.

@sethrj sethrj requested a review from pcanal as a code owner December 5, 2025 15:55
@sethrj sethrj added minor Refactoring or minor internal changes/fixes ci Continuous integration infrastructure labels Dec 5, 2025
@codecov
Copy link

codecov bot commented Dec 5, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 84.90%. Comparing base (8db575e) to head (f05e25a).
⚠️ Report is 2 commits behind head on develop.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##           develop    #2150    +/-   ##
=========================================
  Coverage    84.89%   84.90%            
=========================================
  Files         1273     1273            
  Lines        44667    44667            
  Branches     16765    16611   -154     
=========================================
+ Hits         37922    37926     +4     
+ Misses        4933     4758   -175     
- Partials      1812     1983   +171     

see 132 files with indirect coverage changes

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

@github-actions
Copy link

github-actions bot commented Dec 5, 2025

Test summary

 5 400 files   8 692 suites   17m 30s ⏱️
 2 078 tests  2 052 ✅  26 💤 0 ❌
30 374 runs  30 253 ✅ 121 💤 0 ❌

Results for commit f05e25a.

@pcanal
Copy link
Contributor

pcanal commented Dec 5, 2025

has led to too many people being requested by default as codeowners

Which PR has too many (eg. #2149 is seemingly 'correct' except for the code-lead being added because of the missing coverage for the test directories)?

Copy link
Contributor

@pcanal pcanal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (although I would call this a simplification but rather an extension/correction to cover all top level directories in addition to src)

@sethrj
Copy link
Member Author

sethrj commented Dec 5, 2025

has led to too many people being requested by default as codeowners

Which PR has too many (eg. #2149 is seemingly 'correct' except for the code-lead being added because of the missing coverage for the test directories)?

I think @amandalund ended up deselecting a bunch of irrelevant people for a recent refactor: some of my pulls have shown up with like six people . I wish we could distinguish "give people a heads up that this code is being touched" from "please take a look at this and give me feedback" .

@amandalund
Copy link
Contributor

Yes, I didn't need a review from half the team in #2146 for a minor reorganization... but since it touched files in many different places maybe it's difficult to avoid.

@pcanal
Copy link
Contributor

pcanal commented Dec 5, 2025

I didn't need a review from half the team in #2146 for a minor reorganization...

True but conversely they (maybe) needed to be made aware of the change ... (i.e. Seth's distinction between please review vs please take note).

@sethrj
Copy link
Member Author

sethrj commented Dec 5, 2025

I think "reviewers" should be people who are requested to review the code, since at least one must do so. A secondary function ('pinging' people who have worked recently on the given files) could be implemented with a bot tagging people who have git history with the files (and we could maybe filter on PRs that aren't 'minor').

@sethrj sethrj merged commit bc43daa into celeritas-project:develop Dec 5, 2025
41 of 44 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci Continuous integration infrastructure minor Refactoring or minor internal changes/fixes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants