You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Copy file name to clipboardExpand all lines: CONTRIBUTING.md
+20-7Lines changed: 20 additions & 7 deletions
Display the source diff
Display the rich diff
Original file line number
Diff line number
Diff line change
@@ -655,9 +655,11 @@ This goes hand in hand with [Writing good commit messages](#writing-good-commit-
655
655
656
656
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.
657
657
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.
658
-
Would you immediately know what the code does by glancing at it?
658
+
Would you immediately know what the code does or why it is needed by glancing at it?
659
659
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.
660
660
Doing this preemptively can save you and the committer a lot of time.
661
+
To better convey the "story" of your change, consider dividing your change into multiple atomic commits.
662
+
There is a balance to strike however: over-fragmentation causes friction.
661
663
662
664
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.
663
665
Any individual committer can only really assess components that they themselves know how to use however and yet they must still be convinced somehow.
@@ -685,7 +687,9 @@ Ask them nicely whether they still intend to review your PR and find yourself an
685
687
686
688
### How can I get a committer to look at my PR?
687
689
688
-
- Simply wait. Reviewers frequently browse open PRs and may happen to run across yours and take a look.
690
+
- Improve skimmability: use a simple descriptive PR title (details go in commit titles) outlining _what_ is done and _why_.
691
+
- Improve discoverability: apply all relevant labels, tick all relevant PR body checkboxes.
692
+
- Wait. Reviewers frequently browse open PRs and may happen to run across yours and take a look.
689
693
- Get non-committers to review/approve. Many committers filter open PRs for low-hanging fruit that are already been reviewed.
690
694
-[@-mention](https://github.blog/news-insights/mention-somebody-they-re-notified/) someone and ask them nicely
691
695
- Post in one of the channels made for this purpose if there has been no activity for at least one week
@@ -706,7 +710,7 @@ Don't worry about it.
706
710
If there is a build failure however and it happened due to a package related to your change, you need to investigate it of course.
707
711
If ofBorg reveals the build to be broken on some platform and you don't have access to that platform, you should set your package's `meta.broken` accordingly.
708
712
709
-
When in any doubt, please simply ask via a comment in your PR or through one of the help channels.
713
+
When in any doubt, please ask via a comment in your PR or through one of the help channels.
710
714
711
715
## I received a review on my PR, how do I get it over the finish line?
712
716
@@ -726,6 +730,13 @@ There may be constraints you had to work with which they're not aware of or qual
726
730
727
731
There are some further pitfalls and realities which this section intends to make you aware of.
728
732
733
+
### Aim to reduce cycles
734
+
735
+
Please be prepared for it to take a while before the reviewer gets back to you after you respond.
736
+
This is simply the reality of community projects at the scale of Nixpkgs.
737
+
As such, make sure to respond to _all_ feedback, either by applying suggested changes or argue in favor of something else or no change.
738
+
It wastes everyone time waiting for a couple of days just for the reviewer to remind you to address something they asked for.
739
+
729
740
### A reviewer requested a bunch of insubstantial changes on my PR
730
741
731
742
The people involved in Nixpkgs care about code quality because, once in Nixpkgs, it needs to be maintained for many years to come.
@@ -738,11 +749,11 @@ It is convention to mark review comments that are not critical to the PR as nitp
738
749
As the PR author, you should still take a look at these as they will often reveal best practices and unwritten rules that usually have good reasons behind them and you may want to incorporate them into your modus operandi.
739
750
740
751
Please keep in mind that reviewers almost always mean well here.
741
-
Their intent is not to denounce your code, they simply want your code to be as good as it can be.
752
+
Their intent is not to denounce your code, they want your code to be as good as it can be.
742
753
Through their experience, they may also take notice of a seemingly insignificant issues that have caused significant burden before.
743
754
744
755
Sometimes however, they can also get a bit carried away and become too perfectionistic.
745
-
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.
756
+
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.
746
757
747
758
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.
748
759
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.
@@ -764,9 +775,11 @@ If someone left an approving review on your PR and didn't merge a few days later
764
775
765
776
Please see it as your responsibility to actively remind reviewers of your open PRs.
766
777
767
-
The easiest way to do so is to simply cause them a Github notification.
768
-
Github notifies people involved in the PR when you add a comment to your PR, push your PR or re-request their review.
778
+
The easiest way to do so is to cause them a Github notification.
779
+
Github notifies people involved in the PR whenever you add a comment to your PR, push your PR or re-request their review.
769
780
Doing any of that will get you people's attention again.
781
+
Everyone deserves proper attention, and yes that includes you!
782
+
However please be mindful that committers can sadly not always give everyone the attention they deserve.
770
783
771
784
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.
772
785
Again, this is a community project so please be mindful of people's circumstances here; be nice when requesting reviews again.
0 commit comments