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

lib.options: automatically generate example for booleans #18816

Closed
wants to merge 2 commits into from
Closed

lib.options: automatically generate example for booleans #18816

wants to merge 2 commits into from

Conversation

groxxda
Copy link
Contributor

@groxxda groxxda commented Sep 21, 2016

Motivation for this change

Remove the need to provide an example for boolean options. Since there are only two possibilities, setting example = !default is reasonable. This should remove the amount of boilerplate in nixpkgs by a few hundred lines.

Also catches cases where type == bool but default is not defined and provides example = true.

The second commit catches cases where type != bool but isBool default holds (type may be undefined or something like nullOr bool) and !default is a valid value.
If this is too expensive we can also drop it, but since this is only executed for the manual generation, it should be ok.

The first commit adds 853 example values to options-db.xml, the second one another 3.

ref #18803 could use this 😉

Things done
  • Tested using sandboxing
    (nix.useSandbox on NixOS,
    or option build-use-sandbox in nix.conf
    on non-NixOS)
  • Built on platform(s)
    • NixOS
    • OS X
    • Linux
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Fits CONTRIBUTING.md.

This reduces the boilerplate that is written for boolean options.
We have to check if !default is valid, otherwise
mkOption {
  type = types.enum [ false "x" ];
  default = false;
}
would yield
example = true;
@mention-bot
Copy link

@groxxda, thanks for your PR! By analyzing the annotation information on this pull request, we identified @edolstra, @nbp and @shlevy to be potential reviewers

@chris-martin
Copy link
Contributor

👍 but please also add documentation for these defaults!

@groxxda
Copy link
Contributor Author

groxxda commented Oct 13, 2016

ping @nbp @shlevy do you have an opinion on this?
imo example values for boolean and enum add little value because the type already contains the examples - but since many options provide an example we may as well provide it everywhere.

we could remove all explicit examples from boolean declarations with this pr. i'm not sure if this is wanted though.

Copy link
Member

@nbp nbp left a comment

Choose a reason for hiding this comment

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

I think this is an interesting idea.

Can you submit a page for the documentation such that we can check if reading such documentation is more pleasing?

docOptionExample =
if opt ? example then { example = scrubOptionValue opt.example; }
else if opt.type == lib.types.bool then
{ example = !docOptionDefault.default or false; }
Copy link
Member

Choose a reason for hiding this comment

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

nit: !(docOptionDefault.default or false)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll change that to improve readability.

if opt ? example then { example = scrubOptionValue opt.example; }
else if opt.type == lib.types.bool then
{ example = !docOptionDefault.default or false; }
else if opt ? type && isBool (docOptionDefault.default or null) && opt.type.check (!docOptionDefault.default) then
Copy link
Member

Choose a reason for hiding this comment

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

In opt.type.check (!docOptionDefault.default), the attribute default might not exists, and raise an error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That should never happen since it's only evaluated if isBool (docOptionDefault.default or null)

else if opt.type == lib.types.bool then
{ example = !docOptionDefault.default or false; }
else if opt ? type && isBool (docOptionDefault.default or null) && opt.type.check (!docOptionDefault.default) then
{ example = !docOptionDefault.default or false; }
Copy link
Member

Choose a reason for hiding this comment

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

nit: !(… or …)
nit: factor this expression in a let … in ….

Copy link
Contributor Author

@groxxda groxxda Oct 31, 2016

Choose a reason for hiding this comment

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

I'll check if I can factor that out, but I'd rather not add a let.
Reading it again it's strange I check for opt.type == bool and in the else case check opt ? type. This looks wrong 😕

@joachifm
Copy link
Contributor

IMO, examples for bools contain no useful information. I vote to remove all examples for bools instead.

@ocharles
Copy link
Contributor

I agree with @joachifm, and every example adds 2 lines to man configuration.nix's output. This change would probably be best first run through an RFC. What do people think?

@fpletz
Copy link
Member

fpletz commented Mar 17, 2017

I think there is no sufficient interest in this and we should rather remove the boolean examples. Closing this for now. RFC sounds useful if people want this.

@fpletz fpletz closed this Mar 17, 2017
fpletz added a commit that referenced this pull request Mar 17, 2017
They contain no useful information and increase the length of the
autogenerated options documentation.

See discussion in #18816.
globin pushed a commit that referenced this pull request Mar 18, 2017
They contain no useful information and increase the length of the
autogenerated options documentation.

See discussion in #18816.

(cherry picked from commit 9536169)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants