I saw that there is a ton of unreviewed code in Cpython’s GitHub repo. It seems like the core developers are severely overwhelmed by the amount of work they need to do.
Is there anything us, regular contributors, can help with to alleviate that burden outside of just reviewing code? (which is also a great way to help but it feels like there’s more work than that and I just don’t know about it)
Helping review code is always welcome, but that implies that the review itself is helpful. For example just running the test suite and leaving a LGTM comment is not very helpful; instead that creates more noise, so it ends up being contra-productive. For a review to be helpful, one has to devote time to understand the rationale for the change, and also understand the change itself. If not, it will probably end up being noise.
For example, recently I’ve had reviews of autoconf code where the reviewer clearly does not understand how autoconf works. The review ends up being noise, and I spend time trying to address a moot review. Also, I’ve had reviews of my sqlite3 changes where the reviewer clearly did not understand the rationale for the changes (maybe they did not read up on the issue beforehand? I don’t know). The review ends up being noise.
Perhaps a review checklist would help new reviewers.
In order to not sound too negative; I do highly appreciate a thorough review! But lately, most non-core-dev/non-triager reviews I’ve received have been noise.
Also: If one decides to review code, please also be prepared to follow up review comments. Lately, if I try to address a “noisy” review, the reviewer does not respond back, which is even more frustrating, and thus creating even more noise.
Code review is a skill which takes quite some time to get good at,
and usually requires first having a solid understanding of the
codebase (or relevant subcomponent) for whatever project you’re
reviewing changes on. It’s also a good idea to look through earlier
approved and rejected changes, so that you can see what kinds of
review comments the maintainers find helpful. If you can’t figure
out why certain suggestions were important or unimportant on earlier
changes, then you should refrain from commenting on new changes
until your familiarity with the project’s expectations improves.
The PRs you’re reviewing are “the ton of unreviewed code”. I feel like everybody is welcome to go to Github, check the pull requests and go through them, trying to find problems before a core developer does a review.
But, as Erlend mentioned,
Running the test suite and leaving a LGTM is not very helpful
So if you do want to help, please, follow his recommendations.
… and regarding that specific example: we’ve got CI that run the test suite multiple times, and we’ve got CI that build the docs and check for formatting errors, so there is no need to duplicate those efforts.
As others have said, code review is a multifaceted skill that takes time to hone. One thing to think about, however: rather than just running the test suite, maybe think about things that the tests might have missed. Are there any code paths the PR introduces that look like they might be untested? Is there a bug in the tests that means that they’re not testing what they say they’re testing? Etc etc
Yes, finding code paths without coverage is very helpful! There is no automation for coverage, so this must be done manually. Also, spotting paths that are impossible (for example, this PySomething_Check can be made an assert) is also very helpful. Helping create minimal reproducers for bugs is another very helpful task.
That is nice! A checklist definitely helps. One thing I would add is to check for consistency. Naming conventions, etc. For example, in Path.walk, I have used different variable (not attribute) names in docs and in code, which didn’t affect the user but might’ve made it slightly harder to understand for future readers of the code. We’ve already rectified that, obviously
So I feel like we should watch for such small things too when doing a review. I.e. How consistent are the changes with the rest of the module’s/python’s docs?
I also think a checklist would be very helpful. There is a section in the devguide already, but IMO it could be improved, both for the benefit of reviewers and contributors, both members of the dev teams and other members of the community. A list with clear and actionable items could be an improvement.