Skip to content

Unwritten style guides should be written #387072

@9999years

Description

@9999years

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:

  1. 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-hammering which are not mentioned anywhere in the contribution guide, effectively circumventing the actual Nixpkgs style guidelines.
  2. 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."
  3. Suggestions from different reviewers can contradict each other entirely. For example, a review for #215733 requests that meta = { be changed to meta = with lib; {, while a review for #385393 requests the opposite change.
  4. Suggestions are often low-value (in that they do not impact the resulting derivations produced) or easily automated, like placing passthru before meta in 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.

Metadata

Metadata

Assignees

No one assigned

    Labels

    1.severity: significantNovel ideas, large API changes, notable refactorings, issues with RFC potential, etc.6.topic: architectureRelating to code and API architecture of Nixpkgs6.topic: continuous integrationAffects continuous integration (CI) in Nixpkgs, including Ofborg and GitHub Actions6.topic: developer experiencenixpkgs development workflow9.needs: community feedbackThis needs feedback from more community members.9.needs: documentationThis needs to be documented well.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions