How to help core developers?

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)

2 Likes

I’ve done a bit more research on the topic. Seems like all we can do is review others’ code which might catch some issues early, making the job of core review a bit easier.

1 Like

Hi, you say ‘a ton of unreviewed code’, how did you understand that? How can I find it? And would I be allowed to help?
I am just a contributor who is currently reviewing PRs.

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.

4 Likes

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.

1 Like

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.

3 Likes

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.

2 Likes

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

Absolutely!

2 Likes

A little harsh, but I get your comments and will do better. Being unsupervised and just given a simple directive to go review PRs (because there is a huge number outstanding) is my only lame excuse.

1 Like

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

1 Like

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.

2 Likes

I created a post with suggestions on how to review a recent PR. Does it make sense? Is it worth it? Is it helpful for new reviewers, or is it just more confusing?

C&P of the post here, for those who do not want to chase the link:

Suggestions to how to help reviewing this PR:

  • rewording: improve my English; I’m not a native speaker
  • userfriendlyness: is the rewritten docs more userfriendly or less userfriendly? why? how can it be improved?
  • examples: do the examples make sense? are they easily understood? how can they be improved?
  • brevity/verboseness: is this part of the docs too verbose? or the opposite? are there redundant passages (I said the same thing twice using different wordings)?
  • are there obvious reST markup issues?
2 Likes

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 :slight_smile:

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?

1 Like

Good catch. That’s definitely important.

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.

1 Like

See also prior discussion:

Here’s a different kind of PR. Are the review suggestions helpful? Could something like this be added to the devguide?

2 Likes

Yes! I feel like it’s incredibly detailed and really helps someone learn not only how to review in CPython, but how to review in general.

1 Like

Thanks for the feedback. Perhaps a generalised version could be added to the devguide (and that in itself is a very good task for someone who want to contribute to CPython).

2 Likes

cc. @ezio-melotti

1 Like