How can I be more useful at reviewing pull requests?

Help
2

[quote="turion, post:1, topic:6731]
I like NixOS a lot and I’d like to improve the way how I contribute. Today I had a free day and I decided it was a good opportunity to learn more Nix (I consider myself something between beginner and advanced) and to give something back to nixpkgs/NixOS. I hear that pull request reviews are sought after. Let me describe what questions I typically have when I try to review a pull request.
[/quote]

I hear you. The current state is far from ideal. Let me try to give you some answers, but many of the points you raise are just outstanding issues in the nixpkgs ecosystem.

[quote="turion, post:1, topic:6731]
0. I often don’t know what pull request to pick. What’s a good strategy? Right now, I look for open pull requests that don’t have a review, and sort by least comments. Should I pick new PRs or old ones?
[/quote]

I have the same problem. That is why I originally started [RFC 0030] Formalize review workflow by timokau · Pull Request #30 · NixOS/rfcs · GitHub. Unfortunately I never got around to finishing it. The RFC process has improved a lot since then. If you really have some time on your hands and want a “baptism by fire”, feel free to take the lead on this.

The general idea is that you, as the reviewer, should be able to pick any PR that has a certain tag (needs:review). That PR should be actionable. After the action, you can change the tag. The PR author can change it back to needs:review if they think the PR is ready for another review.

[quote="turion, post:1, topic:6731]

  1. I get the feeling that I shouldn’t review this PR. Numerous reasons can occur:
  • I discover that I know little about the topic being addressed in the PR. Docs about a feature I’ve never used, programming languages I don’t use, packages I’ve never heard about. Is that enough reason to leave the PR to someone else?
    [/quote]

No! You can see this as an opportunity to learn. First improve the PR as much as you can with your knowledge. Then delegate. Dig through git history, look for someone who is an expert. Ping them with a specific question. Most people will be happy to help when pinged and you will learn something.

[quote="turion, post:1, topic:6731]

  • Often, a review was requested from someone (the code owners, I assume), and they haven’t reacted yet, often after a long time. Is the PR “their turf” now? How much value does my review have if someone else who is probably more knowledgeable (after all, they probably wrote/edited the code in question) is supposed to review?
    [/quote]

No! No such thing as their turf. Also one of the motivations behind rfc#30. Do your review. After you think the PR is good to go, ping them again and see if they have another comment. Give them some time, but no need to wait excessively long.

[quote="turion, post:1, topic:6731]

Yes, testing the change is helpful. Although the automated PRs have just recently become so good, barely any manual testing is necessary, there is still nothing that can trump human review. Have a look at the changelog. If its really trivial, just approve it. It makes it easier for people with write access to find low-hanging fruit.

[quote="turion, post:1, topic:6731]

  • Many PR seems really small (say, one line), so I don’t see how my reviewing can help. After all, I don’t have write access, so someone else will still need to merge, and they surely won’t merge without having a look at the very short diff?
    [/quote]

The previous point applies here as well.

[quote="turion, post:1, topic:6731]
2. Let’s say I find a PR where I can actually do something. I make some suggestions, they get adapted. I accept the changes. Great. Now there is a green light next to my github handle, that’s nice. Who will merge this now? Anyone? Ever? Can I set a label “review done” or notify someone?
[/quote]

Solving this was the intention behind PRs already reviewed, although it could be solved even better by rfc#30.

5 Likes