Simplify codeowners

[kratman]

Description

This will simplify the code owners file.

Effects:

• Maintainers requested for all files. Some files do not get any automatic review requests. For instance I had to manually request reviewers on Fix scripts test stability #4849
• Once a single maintainer reviews, the rest of the maintainers are no longer in the list of requested reviewers. This makes it easier to see when specific people were requested by the author
• No need to update a complicated codeowners file when maintainers are added or removed

Type of change

Please add a line in the relevant section of CHANGELOG.md to document the change (include PR #)

Important checks:

Please confirm the following before marking the PR as ready for review:

• No style issues: nox -s pre-commit
• All tests pass: nox -s tests
• The documentation builds: nox -s doctests
• Code is commented for hard-to-understand areas
• Tests added that prove fix is effective or that feature works

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.70%. Comparing base (e39d72e) to head (d174793).
Report is 2 commits behind head on develop.

Additional details and impacted files

@@           Coverage Diff            @@
##           develop    #4850   +/-   ##
========================================
  Coverage    98.70%   98.70%           
========================================
  Files          303      303           
  Lines        23353    23353           
========================================
  Hits         23050    23050           
  Misses         303      303           

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

[Saransh-cpp]

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 😬

[agriyakhetarpal]

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.

[kratman]

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

[agriyakhetarpal]

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).

[Saransh-cpp]

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.

[Saransh-cpp]

Can iterate on this if it does not work. Testing in a fork would be an overkill.

[kratman]

Yeah, in my experience the teams setup reduces pings due to the removal of excess reviewers after the first comment/review. Especially when we tend to start reviews very fast even when PRs end up staying open for a long time.