-
-
Notifications
You must be signed in to change notification settings - Fork 18.1k
Description
Nixpkgs has a collection of style guidelines for writing derivations listed in the CONTRIBUTING.md and pkgs/README.md files. However, these guidelines are far from complete. As a result, Nixpkgs reviewers often take it upon themselves to request changes to comply with their (unwritten, personal) style guides. These suggestions are frustrating for PR authors for several reasons:
- It's impossible to write a PR that complies with these suggestions before receiving a review, because they're not written down anywhere. Some suggestions come from third-party tools like
nixpkgs-hammeringwhich are not mentioned anywhere in the contribution guide, effectively circumventing the actual Nixpkgs style guidelines. - The suggestions are frequently given to version bump PRs which do not touch the code that the suggestion applies to. Simon Tatham describes this as "The Ransom Note" in his "Code Review Antipatterns" article: "This particular patch seems especially important to the developer who submitted it. [...] But this patch isn’t especially vital to you – so you’re in a position of power! Now you can hold the change they need hostage until they do lots of extra tangentially related work, which doesn’t really need to be in the same commit, but which is important to you."
- Suggestions from different reviewers can contradict each other entirely. For example, a review for #215733 requests that
meta = {be changed tometa = with lib; {, while a review for #385393 requests the opposite change. - Suggestions are often low-value (in that they do not impact the resulting derivations produced) or easily automated, like placing
passthrubeforemetain the source code.
I would like to propose several changes to address the proliferation of these comments.
Empowering PR authors to push back
PR authors, particularly less-experienced contributors, should be empowered to push back on unrelated and pedantic suggestions. PR authors should not need a second, less pedantic reviewer to see their changes to avoid this sort of harassment. Changes to code style that are not backed up with a reference to the style guide should be dismissed without being addressed. This will incentivize the implementation of my second suggestion:
Codifying unwritten style guidelines
Style guidelines should be written down so that PR authors can inspect them and do their best to follow them before receiving a review. Style changes that are not written down should be ignored. It should not be possible for individual reviewers to circumvent standard Nixpkgs processes by simply spamming reviews.
Encoding lints in CI
If lints are easy to mechanically detect (like placing passthru before meta in the source code, or omitting preBuild in a buildPhase), there should be automated lints in CI to check these properties automatically. This will free up reviewers to focus more on correctness in their reviews.
Limitations
There will always be review comments which cannot be backed up by a reference to a style guide. However, we want to reduce the amount of these comments as much as possible. While I feel confident we can all agree that certain stylistic changes are in fact style guidelines (like those mentioned elsewhere in this issue), we can work on more subtle suggestions once we've addressed the low-hanging fruit.