How can I be more useful at reviewing pull requests?

Help
1

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.

  1. 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?
  2. 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?
  • 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?
  • Many PRs seem like I’m not supposed to review them. For example, there are many automated version bumps like mtprotoproxy: 1.0.9 -> 1.1.0 by r-ryantm · Pull Request #82430 · NixOS/nixpkgs · GitHub, and I don’t know what I could review there. Am I supposed to test the change? How, maybe nixpkgs-review? Couldn’t a CI do that?
  • 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?
  1. 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?
13 Likes
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
3

@timokau’s answer is good, but I’ll add some further concrete points that helped me get started.

  • Have a quick browse on how people do PRs and reviews. Github’s search is useful for that. For example, here’s my list of PR comments.

    Ultimately we should just write down in detail how the process works, so that anybody can read a doc and know it. As @timokau said, that is still in the making. But even once that texists, looking at real examples is useful.

  • Picking a topic that you are familiar with (independent of Nix) can make it easier to get into the process. For example, assume you familiar with Python. Then you may find it easier to check whether changes to Python programs or libraries work, and might start with those.

  • Similarly, what are programs that you use and like? Just being a user of something makes you very useful, because you can say whether an upgrade of some program works as expected.

    PR yourself as a co-maintainer of such programs (into the maintainers list of the package); then you will be pinged when a new upgrade comes in.

  • If it’s a GUI or a CLI app, just run it and tell if it works. Example:
    Recent PR to upgrade notejot, a note-taking program. I know nothing about the program, but I could start it and see “yup, seems to take notes”. So I wrote:

    GUI works.

    Even those 2 words are extremely useful, because then a committer can come and see “OK, the diff looks good (especially, nonmalicious), and a user reported that the program works, so I can merge”. This frees up significant amount of committer time, so that they have more time for committer tasks (like checking diffs).

  • When a bot tells that some package fails to build (for example here, "1 package failed to build:"), check whether those packages already fail to build on master, and comment accordingly (example comment I made). This tells people whether it can likely be merged anyway (because it’s not a fault of that PR), or whether it should be investigated in more detail.

  • You can read changelogs. Continuing the example above, as a reviewer I commented:

    Changelog looks good and mentions no backwards incompatibilities; the bot has built all dependents.

  • Always state (concisely) what you have done (run this, checked that), so that others know which human-required activity was already done.

The above are general tips for the reviewing process. But there’s more that you can do, to have even higher impact and improve the overall efficiency of the project:

  • Learn how to write NixOS tests (in the tests/ directory), and write some tests for some software you use that doesn’t have tests yet. This will drastically reduce the need for human testing of all further upgrades of that package.

    Even simple tests are useful. For example, if you were to write a test for git, you could make a couple commits and check that they appear in the log output as expected.

9 Likes
4

Thanks @timokau and @nh2 for your helpful answers! I have the feeling that I could already contribute better now, and I (hopefully helpfully) reviewed a longer PR and contributed some tests.

At this point, I also just want to thank @zimbatm from whom I have learned a lot about nix and nixpkgs in his Twitch video stream. Without that, I think I wouldn’t even be in the position to seriously considering doing this.

3 Likes