Skip to content
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.

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

nixos/*: add trivial defaultText to options where applicable #147441

Merged
merged 3 commits into from
Dec 6, 2021
Merged

nixos/*: add trivial defaultText to options where applicable #147441

merged 3 commits into from
Dec 6, 2021

Conversation

pennae
Copy link
Contributor

@pennae pennae commented Nov 26, 2021

"trivial" here means "can just copy the expression, add quotes and it's useful". this mostly means that the default expressions given don't use shortcuts like the ubiquitous cfg = config.${our module name}, runs data through a function not in the default expression, or just anything that can't be read in isolation without knowing the context.

working towards caching most of the option documentation to improve build times. this change might speed up the build a little already, but further changes that improve this even more are upcoming.

we have a good few more commits that add defaultTexts to the remaining options that use cfg shortcuts in their defaults or more complicated expressions, will open a PR with those once this is merged (unless there's wishes to include those changes here). after that we'll probably have some more changes (based on #147265) that make most of the option docs for nixos/modules cacheable.

one commit per module by request of @samueldr :)

@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Nov 26, 2021
@pennae pennae requested a review from samueldr November 26, 2021 01:00
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Nov 26, 2021
@samueldr
Copy link
Member

samueldr commented Nov 26, 2021

Note that I originally suggested per-module commits for modules with non-trivial changes (which would include the trivial changes for said module too). For trivial only changes, maybe some grouping in bite-sized chunks.

I did also say that if it was my opinion only, I would do it just like this was done now, so I don't mind personally, I only know other contributors, maintainers and devs don't like a flood of discrete commits :).

@pennae pennae requested a review from jtojnar December 1, 2021 16:15

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Copy link
Member

@ckiee ckiee left a comment

Choose a reason for hiding this comment

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

I looked at all the diffs and checked the more odd ones out in my local build of the manual. Just one~

Edit: Usually the commit messages for stuff like this are prefixed with treewide:, not nixos/*:.

nixos/modules/services/monitoring/smartd.nix Show resolved Hide resolved
@Artturin Artturin requested a review from roberth December 5, 2021 23:18
@roberth
Copy link
Member

roberth commented Dec 5, 2021

Please merge when ofborg is successful. 🚀

@pennae
Copy link
Contributor Author

pennae commented Dec 5, 2021

Please merge when ofborg is successful. 🚀

we don't have commit permissions, so someone else will have to :D

@roberth roberth merged commit 862d167 into NixOS:master Dec 6, 2021
@jtojnar
Copy link
Member

jtojnar commented Dec 6, 2021

Usually the commit messages for stuff like this are prefixed with treewide:, not nixos/*:.

Note that we also have a style rule that NixOS changes should be prefixed by nixos/: https://nixos.org/manual/nixpkgs/stable/#submitting-changes-making-patches

@ckiee
Copy link
Member

ckiee commented Dec 6, 2021

Note that we also have a style rule that NixOS changes should be prefixed by nixos/: https://nixos.org/manual/nixpkgs/stable/#submitting-changes-making-patches

Mhm, so a treewide NixOS change is kind of undefined behavior. I've created a (probably naive) PR to fix that: #148996

@pennae pennae deleted the option-doc-staticizing branch December 7, 2021 20:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants