Skip to content

CONTRIBUTING.md: discourage nitpicking #336200

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
pbsds wants to merge 1 commit into from
Closed

CONTRIBUTING.md: discourage nitpicking #336200

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CONTRIBUTING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -317,6 +317,8 @@ GitHub provides reactions as a simple and quick way to provide feedback to pull

Pull request reviews should include a list of what has been reviewed in a comment, so other reviewers and mergers can know the state of the review.

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.
Copy link
Contributor

@MattSturgeon MattSturgeon Sep 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@MattSturgeon MattSturgeon Sep 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@pbsds pbsds Sep 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)".

Copy link
Contributor

@MattSturgeon MattSturgeon Sep 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@pbsds pbsds Sep 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@MattSturgeon MattSturgeon Sep 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@emilazy emilazy Sep 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Copy link
Member Author

@pbsds pbsds Sep 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@Aleksanaa Aleksanaa Sep 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@Aleksanaa Aleksanaa Sep 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@pbsds pbsds Sep 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


All the review template samples provided in this section are generic and meant as examples. Their usage is optional and the reviewer is free to adapt them to their liking.

To get more information about how to review specific parts of Nixpkgs, refer to the documents linked to in the [overview section][overview].
Expand Down