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.

Sign up for GitHub

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

Simplify codeowners #4850

Merged
merged 2 commits into from
Feb 16, 2025
Merged

Simplify codeowners #4850

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8e42ff6
Simplify codeowners
kratman Feb 14, 2025
d174793
Merge branch 'develop' into chore/fixMaintainers
kratman Feb 16, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 1 addition & 33 deletions .github/CODEOWNERS
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,34 +1,2 @@
# Automatically request reviews from maintainers

# Package
src/pybamm/discretisations/ @martinjrobins @rtimms @valentinsulzer
src/pybamm/experiment/ @brosaplanella @martinjrobins @rtimms @valentinsulzer @TomTranter
src/pybamm/expression_tree/ @martinjrobins @rtimms @valentinsulzer
src/pybamm/geometry/ @martinjrobins @rtimms @valentinsulzer
src/pybamm/input/ @brosaplanella @DrSOKane @rtimms @valentinsulzer @TomTranter @kratman
src/pybamm/meshes/ @martinjrobins @rtimms @valentinsulzer @rtimms
src/pybamm/models/ @brosaplanella @DrSOKane @rtimms @valentinsulzer @TomTranter @rtimms
src/pybamm/parameters/ @brosaplanella @DrSOKane @rtimms @valentinsulzer @TomTranter @rtimms @kratman
src/pybamm/plotting/ @martinjrobins @rtimms @Saransh-cpp @valentinsulzer @rtimms @kratman @agriyakhetarpal
src/pybamm/solvers/ @martinjrobins @rtimms @valentinsulzer @TomTranter @rtimms @MarcBerliner
src/pybamm/spatial_methods/ @martinjrobins @rtimms @valentinsulzer @rtimms
src/pybamm/* @pybamm-team/maintainers # the files directly under /pybamm/, will not recurse

# CI/CD workflows
/.github/ @martinjrobins @Saransh-cpp @agriyakhetarpal @kratman @arjxn-py

# Benchmarks
/benchmarks/ @brosaplanella @Saransh-cpp @agriyakhetarpal @arjxn-py

# Documentation
/docs/ @kratman @arjxn-py @agriyakhetarpal @Saransh-cpp

# Example scripts
/examples/ @kratman @agriyakhetarpal @Saransh-cpp

# Installation and other scripts
/scripts/ @martinjrobins @Saransh-cpp @agriyakhetarpal @kratman @arjxn-py

# Files in the root directory
/* @martinjrobins @Saransh-cpp @agriyakhetarpal @kratman @arjxn-py
/CHANGELOG.md # no owner (almost every PR edits the CHANGELOG)
* @pybamm-team/maintainers
Copy link
Member

Choose a reason for hiding this comment

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

Every maintainer might not want a notification/mail for every new PR.

I never added the tests/ folder here because I never thought of PRs just editing test cases (which is very much possible). An alternative solution here would be to do the same with tests/ folder as we do with pybamm/. Another option would be to remove CODEOWNERS as it hasn't really been very helpful.

We can also turn off notifications for a team - https://docs.github.com/en/organizations/organizing-members-into-teams/configuring-team-notifications - but I guess that defeats the purpose of requesting a review. I would personally not want a notification/mail for every PR 😬

Copy link
Member

Choose a reason for hiding this comment

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

Another option would be to remove CODEOWNERS as it hasn't really been very helpful.

Yeah, CODEOWNERS has been usually messy. If a file is touched that may or may not be relevant to the other changes and the PR is not a draft, everyone gets pinged. It might also be redundant, as most of the maintainers are watching the repository anyway.

I would be curious if we could explore more labels for components, such as how SciPy does it, so that maintainers in their free time can take a look at the issues/PRs they are interested in by the use of labels. But at the same time, SciPy uses CODEOWNERS as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The way codeowners should work is that everyone on the team gets requested for review, but then once a single reviewer comments/reviews, the rest of the reviewers are removed and only the person that commented is on there. Right now anyone marked for a file/directory stays on the PR.

The way I have it setup with a single line should be both simpler and less pings than we have now.

It works even better if everyone makes their PRs drafts. Because code owners should not be pinged until it is marked as ready for review.

At least this is the way it worked back in 2020

Copy link
Member

Choose a reason for hiding this comment

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

It works even better if everyone makes their PRs drafts. Because code owners should not be pinged until it is marked as ready for review.

At least this is the way it worked back in 2020

Yes, it is the same even now. We can't expect new contributors to start their PRs as drafts even if we ask them to, but we could play our part and follow this.

but then once a single reviewer comments/reviews, the rest of the reviewers are removed and only the person that commented is on there

That sounds like a good feature request. But is there a better solution than the one in this PR? It will still ping all code owners on all PRs, and I don't think it will reduce the number of pings (but rather increase them – I would still receive notifications even if I decide to unwatch the repository).

Copy link
Member

@Saransh-cpp Saransh-cpp Feb 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, draft PRs should be encouraged, I agree, but it can't be enforced.

We should really test this in a fork once. For instance, I don't watch the repository so the number of pings for me will go up. But again, it might go down depending on if GitHub unsubscribes me from the PR as soon as someone reviews it.

Could someone request a maintainer review in #4849 again? @agriyakhetarpal can then approve it and make a comment after the approval. We can decide how to proceed ahead depending on the number of mails I get 😆

Edit: I guess we can always iterate on this. If it does not work we can revert back, so should not be a huge peoblem.