-
-
Notifications
You must be signed in to change notification settings - Fork 18.6k
Unwritten style guides should be written #387072
Copy link
Copy link
Open
Labels
1.severity: significantNovel ideas, large API changes, notable refactorings, issues with RFC potential, etc.Novel ideas, large API changes, notable refactorings, issues with RFC potential, etc.6.topic: architectureRelating to code and API architecture of NixpkgsRelating to code and API architecture of Nixpkgs6.topic: continuous integrationAffects continuous integration (CI) in Nixpkgs, including Ofborg and GitHub ActionsAffects continuous integration (CI) in Nixpkgs, including Ofborg and GitHub Actions6.topic: developer experiencenixpkgs development workflownixpkgs development workflow9.needs: community feedbackThis needs feedback from more community members.This needs feedback from more community members.9.needs: documentationThis needs to be documented well.This needs to be documented well.
Metadata
Metadata
Assignees
Labels
1.severity: significantNovel ideas, large API changes, notable refactorings, issues with RFC potential, etc.Novel ideas, large API changes, notable refactorings, issues with RFC potential, etc.6.topic: architectureRelating to code and API architecture of NixpkgsRelating to code and API architecture of Nixpkgs6.topic: continuous integrationAffects continuous integration (CI) in Nixpkgs, including Ofborg and GitHub ActionsAffects continuous integration (CI) in Nixpkgs, including Ofborg and GitHub Actions6.topic: developer experiencenixpkgs development workflownixpkgs development workflow9.needs: community feedbackThis needs feedback from more community members.This needs feedback from more community members.9.needs: documentationThis needs to be documented well.This needs to be documented well.
Fields
Give feedbackNo fields configured for issues without a type.
Nixpkgs has a collection of style guidelines for writing derivations listed in the
CONTRIBUTING.mdandpkgs/README.mdfiles. 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:nixpkgs-hammeringwhich are not mentioned anywhere in the contribution guide, effectively circumventing the actual Nixpkgs style guidelines.meta = {be changed tometa = with lib; {, while a review for #385393 requests the opposite change.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
passthrubeforemetain the source code, or omittingpreBuildin abuildPhase), 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.