CONTRIBUTING.md: discourage nitpicking

[pbsds]

Description of changes

Things done

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
  • sandbox = relaxed
  • sandbox = true
• Tested, as applicable:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
• Tested basic functionality of all binary files (usually in ./result/bin/)
• 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
  • (Package updates) Added a release notes entry if the change is major or breaking
  • (Module updates) Added a release notes entry if the change is significant
  • (Module addition) Added a release notes entry if adding a new NixOS module
• Fits CONTRIBUTING.md.

---

Add a 👍 reaction to pull requests you find important.

[nixos-discourse]

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/satisfaction-survey-from-the-new-rfc-166-formatting/49758/42

[Atemu]

See also #315337 (comment)

[pbsds]

We could enumerate a list of non-blockers, they are plentiful, but I'm not sure if that section is the correct place.

[Atemu]

Another thing I've recently started doing is to simply PR my nitpicks against the contributor's branch or even just push them directly if you're a committer (most people leave the "allow maintainers to push commits" setting on in PRs).

I do find it hard to "outlaw" nitpicks like this however. Sometimes, nitpicks do have merit like asking for code comments explaining patches or unusual code. You could argue that those aren't nitpicks though I guess..

[pbsds]

I usually avoid pushing to contributor forks, and sending a PR to their fork usually results in a merge commit they have to clean up.

Most of the time I add code suggestions, sometimes if it is only stylistic issues or some poor but benign use of pname, I let it slide and leave it for cleanup treewides. If I discover a unrelated issue while reviewing something, prefer not to stray from the scope of the PR and rather go and fix it in a separate PR.

[Atemu]

What I also prefer to do these days is approve the PR with a set of nitpicks. By doing this, I try to convey that I'd accept the PR as is but would prefer the nitpicks to be solved. Usually people fix them the next day and I merge but if they don't it should be merged as is a few days later. Perhaps we should establish that as a standard so that other committers can just merge when they see the latter case.

I wish we had some sort of automation that would queue a merge for a week from now if nothing changes. (If the contributor made changes in the mean time, I'd get pinged and can just merge manually.)

[SomeoneSerge]

Another thing I've recently started doing is to simply PR my nitpicks against the contributor's branch or even just push them directly if you're a committer

+1, and I think that as long as you don't force-push it's not particularly intrusive

[pbsds]

For those interested, i've started proposing patches in comments (example). This proved easy to script with git diff --staged, gh gist create, $EDITOR and gh pr review --body-file

[pbsds]

Suggested change

	Pull request reviews that only nitpick things that are easily fixed in treewides cause unnecessary friction and are better left unposted. Nitpicks may however accompany blockers.
	Pull request reviews that only nitpick things that are stylistic in nature or easily fixed in treewides cause unnecessary friction and are better left unposted. Nitpicks may however accompany blockers.

?

[Atemu]

Perhaps "opinionated" would be a better word as we do want to allow change requests for small things that have tangible technical merit for the change at hand but not "you should use pattern x here rather than pattern y" because you prefer pattern y.

[infinisil]

Nitpicks may however accompany blockers.

This creates the weird incentive to find a blocker to justify many nitpicks.

I like C4's guideline on this:

14. Maintainers SHALL NOT make value judgments on correct patches.

18. Any Contributor who has value judgments on a patch SHOULD express these via their own patches.

In general I'd like to move more towards C4, which is well elaborated here (highly recommended reading).

[pbsds]

This is a great resource, i'll add it to my reading.

This creates the weird incentive to find a blocker to justify many nitpicks.

Weird incentive to improve the review quality?

[emilazy]

I have some thoughts about committer vs. contributor time and activation energy and the context switch overhead of reviewing vs. patching here that make me worry that the expectation to provide all not‐absolutely‐critical feedback as separate PRs from the reviewer will be a net drag on maintainer resources over time, but they’re not fully‐formed enough for me to express them other than by gesturing broadly at them here. (This is, admittedly, probably partly a workflow problem. But we’re stuck on GitHub for now…)

[SomeoneSerge]

Some random ideas, mix'n'matched

Suggested change

	Pull request reviews that only nitpick things that are easily fixed in treewides cause unnecessary friction and are better left unposted. Nitpicks may however accompany blockers.
	Pull request reviews that only ask for non-functional ("cosmetic", "stylistic", "meta") changes ("nitpicks") are better left unposted to reduce friction and avoid stalling the PR. Decisions about stylistic changes are better made in global, "treewide", commits and enforced using CI. A "nitpick" is never a blocker, but may accompany a blocker.

I'm also pondering the thought of elaborating "Decisions ... are better made in global" with "if they are worth the making"

[SomeoneSerge]

Suggested change

	Pull request reviews that only nitpick things that are easily fixed in treewides cause unnecessary friction and are better left unposted. Nitpicks may however accompany blockers.
	Be considerate about posting reviews that only ask for changes that are stylistic in nature or easily fixed in treewides, so as to avoid stalling the PR.

[SomeoneSerge]

Probably worth experimenting with the wording some more, but I like the intent

CONTRIBUTING.md: Nitspkgs -> Nixpkgs

[zimbatm]

Thanks for moving nixpkgs to a better culture

[pbsds]

Sorry for the delay, I've been traveling. ✈️

I adapted @SomeoneSerge's second suggestion. I would love to get "value judgement" and "nitpick" into there for the sake of grep-ability, but the current change I believe most can agree on.

[zimbatm]

LGTM

[MattSturgeon]

Suggested change

	Please be considerate about posting reviews that _only_ ask for changes that are stylistic in nature or easily fixed in treewides, so as to avoid stalling the PR.
	Please be considerate about posting review comments that _only_ ask for changes that are stylistic in nature or easily fixed in treewides, so as to avoid stalling the PR.

Suggested change

	Please be considerate about posting reviews that _only_ ask for changes that are stylistic in nature or easily fixed in treewides, so as to avoid stalling the PR.
	Please be considerate about posting feedback that _only_ ask for changes that are stylistic in nature or easily fixed in treewides, so as to avoid stalling the PR.

I think this should relate to review comments or feedback more generally.

Talking about reviews as a whole could imply we're ok with nit-picking when included as part of a larger review.

Taking this further, it could even incentivise artificially inflating the scope of the review, as a means to justify posting less important feedback.

[MattSturgeon]

Perhaps it should also be specifically talking about comments/feedback that request changes?

I can't speak for others, but personally I'm happy to give or receive feedback that takes the form "you don't need to change this, but".

E.g. "This isn't important here, but as an FYI the best practice is usually ..."

Or "This is purely stylistic, so you're welcome to ignore: if you do ... then nixfmt will handle this better."

I see these as friendly advice or tips, and I don't think we should demonise this. At least so long as it's made clear that such feedback doesn't need to be addressed in order to merge a PR.

[pbsds]

I'm not advocating for a total ban on discussing style, I only want to reduce the number of review cycles. Hence the emphasis on "only".

If a find a mergeable PR where a comment/review asks for a change without explicitly stating they consider it a non-blocker, which happens often, I tend to leave it.
This is because ignoring such a comment and merging the PR might by some be viewed as a slight, and I'm not the kind of person who likes to risk ruffling someones feathers.

Asking reviewers to explicitly note all their nits as non-blockers is a Sisyphean task, since the alternative is laziness.

incentivise artificially inflating the scope of the review

If this incentivizes a more thorough review, then I personally consider that a win.
If the reviewer decides to block on some pre-existing issue that is not thematically related to the current PR, I consider that to be holding the PR hostage, which I feel perfectly safe dismissing with a "PR welcome :)".

[MattSturgeon]

I'm not advocating for a total ban on discussing style, I only want to reduce the number of review cycles. Hence the emphasis on "only".

I understand and agree with your intent. It's how the current wording could be interpreted that concerns me, not how it was intended.

I think the emphasis should not be put on whether a nitpick is the "only" feedback; rather I think the wording should focus on how "forceful" the nitpick is.

If the reviewer decides to block on some pre-existing issue that is not thematically related to the current PR, I consider that to be holding the PR hostage, which I feel perfectly safe dismissing with a "PR welcome :)".

IMO focusing on how forceful the nitpick is could also serve to discourage such ransom notes, reducing the number of PRs that need to be dismissed. It also provids a line in CONTRIBUTING that can be referenced when addressing such ransom notes.

[pbsds]

I agree, and if everyone did as you do during review we would not have the problems we have today. The problem is that the path of least resistance when reviewing is to be as brief as possible. This often means (1) select offending line, (2) click "add suggestion", (3) make changes to offending line, (4) submit. Is that review forceful? Is it a non-blocking nit? It is entirely up to the reader to interpret what the code change suggestion is trying to communicate. If the avatar of the reviewer by chance has a frowny face, are they being forceful? You start grasping at straws.

I fail to see how your two suggestions address forcefulness. Could it be improved?

[MattSturgeon]

I fail to see how your two suggestions address forcefulness.

Correct, my initial suggestions only changed the focus from reviews as a whole to individual review comments.

It was on reflection that I began thinking about how this also relates to how forceful the feedback is.

Could it be improved?

Perhaps we can iterate on something along these lines?

Suggested change

	Please be considerate about posting reviews that _only_ ask for changes that are stylistic in nature or easily fixed in treewides, so as to avoid stalling the PR.
	Please be considerate about posting feedback that _only_ ask for changes that are stylistic in nature or easily fixed in treewides, so as to avoid stalling the PR.
	If you insist on posting such feedback, please state clearly that it is a suggestion which should not block merging.

[emilazy]

I think “reviews” here was a deliberate compromise, see e.g. #336200 (review).

While I’m guessing nobody is entirely happy with the current wording, it seems to me to have the useful property that nobody who has commented would strongly object to it, and that it clearly discourages the most pathological review styles while leaving room for differing opinions. I do think “but educating people about conventions is okay if you make it clear that it’s not a hard blocker” wouldn’t go amiss, but if that’s going to stall this further I’d rather avoid the… uh… nitpicking :)

[pbsds]

The suggestion adds an escape hatch that will IMO only serve to work against the goal reducing review cycles. If a reviewer does post a nit review, what you're then left to argue is its lack of pleasantries to satisfy guidelines, which will in turn spark further arguments.

I'd rather we redirect the nit energy to something more productive, like "express the desired changes in your own PRs.". But adding this to the guideline makes it wordy and unwieldy.

[Aleksanaa]

The problem is, I suppose, we all know that there are implicit style guidelines, some of which are drawn from mistakes that are easy to make, but most of them are neither written clearly nor detected automatically (with CI and local tooling).

I can name a few:

1. meta = with lib; { ... }: We didn't state that this is bad in our documentation, but only removed examples from it.
2. let ... in mkDerivation { inherit ... } vs mkDerivation rec { ... }: We even don't have unified opinions towards this?
3. rev = "refs/tags/${version}": Where did we document the reason and when to use?
4. We all know that there is an order in packaging attributes, such as preBuild should be after patches, passthru should be before meta, nativeBuildInputs should be before buildInputs. But where did we document this?

The solution would be creating a set of samples to learn and copy from, and write clear style guides.

And for the "stall" part. The most problematic thing is not giving stylistic suggestions, but giving incomplete and unsubstantiated (and therefore may appear randomly) suggestions. Requiring repeated revisions is the main cause of the problem. I don't think it is a bad thing to give stylistic suggestions in the first requesting change, because the contributor should foresee the possibility of requesting changes and have the ability to handle such requests.

Like if I write this PR:

A problematic PR

{
  fetchFromGitHub,
  lib,
  stdenv,
}:

let
  owner = "velorek1";
  repo = "C-edit";
  rev = "970790262df07448d9d283634c43626743e14438";
  hash = "sha256-Nc4lwRmjPpl8cWp5di7tboqQuhAqhw43rs/+HZoLyY4=";
  pname = "c-edit";
  version = "0-unstable-2024-09-01";

  src = fetchFromGitHub {
    inherit owner repo rev hash;
  };

  installPhase = ''
    runHook preInstall
    install -Dm755 ./cedit -t $out/bin
    ln -s $out/bin/cedit $out/bin/c-edit
    runHook postInstall
  '';
in

stdenv.mkDerivation {
  inherit pname version src;

  sourceRoot = "${finalAttrs.src.name}/src";

  meta = with lib; {
    description = "Text editor in the style of the MSDOS EDIT, without using ncurses";
    homepage = src.meta.homepage + "/tree/master";
    license = with licenses; [ mit ];
    mainProgram = "cedit";
    maintainers = [ maintainers.aleksana ];
    platforms = with platforms; linux;
  };
})

This is largely correct despite of huge stylistic non-conformist.

[Aleksanaa]

(I'm not requesting changes to this, just open a review to ease tracking discussion)

[pbsds]

Just because a handful of prolific contributors has taken upon themselves to push through these preferences via a bunch of treewide PRs and reviews doesn't mean that these preferences are necessarily wanted by the larger community, nor even acknowledged as actual improvements. The lack of documentation of these preferences is in part due to a lack of consensus. One of the beauties of nixpkgs IMO is how it is an incredible pool of domain knowledge where each ecosystem has developed their own preferences and styles that suit their needs.

We all know that there is an order in packaging attributes

(emphasis mine) This is debatable.
There is set execution order, yes, but there are other factors to consider, like for example code locality. Some prefer to list all inputs right below src, while others prefer to interlace them between the *Phase scripts. Because there is no established consensus on this, attribute-ordering is the first rule in nixpkgs-hammer I disable, even though I myself have a strong preference for how to order the attributes.

[pbsds]

done in #264651