The footer has the following problems:
- It's not consistently added in all PRs, for example backports or those
where the template is not used. Inconsistency in a voting skews the
results.
- It's arguably not effective, because people use reactions naturally to
interact with PRs anyway. As long as there are multiple different
reaction types, what does it mean that one PR has 20 👍, but another
has 22 🎉? The latter won't show up in the ranking for 👍, so at
the minimum confusing. Same for ❤️.
- It's not useful to determine priority as in "higher number of votes
should be merged". A PR's quality should be determined independently of
votes - when it's good, it will be merged. The currently most upvoted PR
has serious issues, so this invites drive-by questions like "Why isn't
this merged, given the number of upvotes?".
This doesn't really change the items, but shortens the writing a lot,
making them much more readable, especially while still drafting the PR.
Mentioning the release notes for the previous release is not really
important, because it doesn't apply for the big majority of pull
requests.
This *is* important, but is a niche detail which belongs into the
contributions guidelines - and not into the PR template. It's also out
of place for "what did you test?".
We expect the TODO list to be read through *on PR creation*, otherwise
the html comments would not make sense. Thus, we should make it even
only slightly readable, which was not at all the case before.
The links for release notes are removed, because the PR author has no
value from the *current* release notes. They will need to find the file
manually anyway.
While this note is important, it's also mostly invisible at this stage.
The comment only shows while creating the PR, but at this stage the
author really has other things to worry about.
If they care, they will read the contribution guidelines and will pick
up the pieces about reviewing that way. If they don't - they won't be
bothered by this notice either.
This checkbox has been used very incosistently and its meaning is not
entirely clear: What does it mean if both checkboxes are *unchecked*?
Does that mean the sandbox was disabled? Or does it mean the checkbox
was just not handled?
Also the new nixpkgs-review-gha, which is increasingly used to test
builds on darwin platforms shows this information as part of the review
- where it's in a much better place.
When creating a PR from a branch that only adds a single commit, this heading
would always jankily be left *below* the actual commit message because github
simply inserts the commit message before the template.
The heading also only served as a light call to action whereas the comment is
rather explicit in asking the PR author to provide a proper PR description.
Where this was a markdown heading, it sometimes had the wrong weight
so it appeared as if it was a subheading of the previous section, and
some people feel the word "Priorities" creates misleading
expectations.
Link: https://github.com/NixOS/nixpkgs/pull/203969/files#r1037761779
Update the sandboxing check to include the `relaxed` setting for
sandboxing. Previously there was no obvious and correct way to convey
this intermediate setting between sandboxing being completely disabled
and being enforced strictly.
The original Markdown ended up being rendered as `<H6>` HTML elements,
which is not semantically valid immediately after an `<H1>` element (the
PR title).
This improves URL previews like the ones on Discourse, where currently
the boilerplate comment is printed for every PR link instead of parts of
the motivation.
There are a lot of PRs for updates that don't make it easy to find
out what changes might be breaking and lots of PRs for new packages
that don't describe what the new packages is or does.
1) Building with the sandbox enabled is the standard on Linux,
so it is of little relevance with distribution they are using.
Because of this, we now specify "Linux" instead of "NixOS",
and the sandbox checkbox specifies "non-Linux".
2) aarch64 is a much more common platform, so we add two separate
checkboxes for both aarch64-linux and aarch64-darwin.
3) The platform names now match what is actually used by Nix.
Remove elements of the PR template that have a low signal/noise ratio,
and add one that I think would have a good signal/noise ratio.
-----
Remove:
Determined the impact on package closure size (by running `nix path-info
-S` before and after)
-----
Rationale:
This is rarely done in practice, and apart from for specific packages
this is usually not a good indicator of anything useful
It might make sense to re-introduce it with two holes to fill, but then
we would have to make a serious decision to never land without these two
numbers filled in or with too big a regression, because in practice this
box has been a no-op in many cases.
Maybe just integrating this check in nixpkgs-review would bring the most
benefit here?
-----
-----
Remove:
Ensured that relevant documentation is up to date
-----
Rationale:
This is fuzzy, “relevant documentation” is way too often hard to find
-----
-----
Add:
Added a release notes entry if the change is major or breaking
-----
Rationale:
This is way too often forgotten, and is also a self-contained easy task
-----
Emily