From dcc3d17ef45e23e9cedfe5f69cff64bf79f37b21 Mon Sep 17 00:00:00 2001 From: Peder Bergebakken Sundt Date: Mon, 21 Oct 2024 13:59:36 +0200 Subject: [PATCH] CONTRIBUTING.md: extend section on getting your PR merged --- CONTRIBUTING.md | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 70e45094f269..9293dc11e7e6 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -659,9 +659,11 @@ This goes hand in hand with [Writing good commit messages](#writing-good-commit- For the code quality assessment, you cannot do anything yourself as only the committer can do this and they already have your code to look at. In order to minimise the need for back and forth though, do take a look over your code changes yourself and try to put yourself into the shoes of someone who didn't just write that code. -Would you immediately know what the code does by glancing at it? +Would you immediately know what the code does or why it is needed by glancing at it? If not, reviewers will notice this and will ask you to clarify the code by refactoring it and/or adding a few explanations in code comments. Doing this preemptively can save you and the committer a lot of time. +To better convey the "story" of your change, consider dividing your change into multiple atomic commits. +There is a balance to strike however: over-fragmentation causes friction. The code artefacts are the hardest for committers to assess because PRs touch all sorts of components: applications, libraries, NixOS modules, editor plugins and many many other things. Any individual committer can only really assess components that they themselves know how to use however and yet they must still be convinced somehow. @@ -689,7 +691,9 @@ Ask them nicely whether they still intend to review your PR and find yourself an ### How can I get a committer to look at my PR? -- Simply wait. Reviewers frequently browse open PRs and may happen to run across yours and take a look. +- Improve skimmability: use a simple descriptive PR title (details go in commit titles) outlining _what_ is done and _why_. +- Improve discoverability: apply all relevant labels, tick all relevant PR body checkboxes. +- Wait. Reviewers frequently browse open PRs and may happen to run across yours and take a look. - Get non-committers to review/approve. Many committers filter open PRs for low-hanging fruit that are already been reviewed. - [@-mention](https://github.blog/news-insights/mention-somebody-they-re-notified/) someone and ask them nicely - Post in one of the channels made for this purpose if there has been no activity for at least one week @@ -730,6 +734,13 @@ There may be constraints you had to work with which they're not aware of or qual There are some further pitfalls and realities which this section intends to make you aware of. +### Aim to reduce cycles + +Please be prepared for it to take a while before the reviewer gets back to you after you respond. +This is simply the reality of community projects at the scale of Nixpkgs. +As such, make sure to respond to _all_ feedback, either by applying suggested changes or argue in favor of something else or no change. +It wastes everyone time waiting for a couple of days just for the reviewer to remind you to address something they asked for. + ### A reviewer requested a bunch of insubstantial changes on my PR The people involved in Nixpkgs care about code quality because, once in Nixpkgs, it needs to be maintained for many years to come. @@ -746,7 +757,7 @@ Their intent is not to denounce your code, they simply want your code to be as g Through their experience, they may also take notice of a seemingly insignificant issues that have caused significant burden before. Sometimes however, they can also get a bit carried away and become too perfectionistic. -If you feel some of the requests are unreasonable or merely a matter of personal preference, try to nicely remind the reviewers that you may not intend this code to be 100% perfect or that you have different taste in some regards and press them on whether they think that these requests are *critical* to the PR's success. +If you feel some of the requests are unreasonable, out of scope, or merely a matter of personal preference, try to nicely remind the reviewers that you may not intend this code to be 100% perfect or that you have different taste in some regards and press them on whether they think that these requests are *critical* to the PR's success. While we do have a set of [official standards for the Nix community](https://github.com/NixOS/rfcs/), we don't have standards for everything and there are often multiple valid ways to achieve the same goal. Unless there are standards forbidding the patterns used in your code or there are serious technical, maintainability or readability issues with your code, you can insist to keep the code the way you made it and disregard the requests. @@ -771,6 +782,8 @@ Please see it as your responsibility to actively remind reviewers of your open P The easiest way to do so is to simply cause them a Github notification. Github notifies people involved in the PR when you add a comment to your PR, push your PR or re-request their review. Doing any of that will get you people's attention again. +Everyone deserves proper attention, and yes that includes you! +However please be mindful that committers can sadly not always give everyone the attention they deserve. It may very well be the case that you have to do this every time you need the committer to follow up upon your PR. Again, this is a community project so please be mindful of people's circumstances here; be nice when requesting reviews again.