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: add prime variants for the rest of defaultNullOpts #1619

Merged

Conversation

MattSturgeon
Copy link
Member

@MattSturgeon MattSturgeon commented Jun 2, 2024

Follow up to #1603

  • plugins: normalise null plugin-defaults
    This first commit was needed to satisfy the assertion I added to mkEnum'.
    I could move this to another PR if needed.
    We could also check each of the files touched to see if other changes ought to be made.

  • lib/options: add more defaultNullOpts 'variants
    The meat of this PR - add a prime variant for each defaultNullOpts option factory function.

  • lib/options: add mkEnum' argument assertions
    I also added some assertions to ensure mkEnum' is always used correctly. This could be kept or dropped (after running it through CI) depending on preference.

@GaetanLepage It'd be great if you could run CI with these changes on #1614.

Replaced all instances of `"null"` with `null`, when passing
plugin-defaults to `defaultNullOpts` functions.
All `defaultNullOpts` functions now have a prime variant.
Comment on lines +80 to +92
let
# Convert `defaultNullOpts`-style arguments into normal `mkOption`-style arguments,
# i.e. moves `default` into `description` using `defaultNullOpts.mkDesc`
convertArgs =
{ default, description, ... }@args:
(
args
// {
default = null;
description = defaultNullOpts.mkDesc default description;
}
);
in
Copy link
Member Author

@MattSturgeon MattSturgeon Jun 2, 2024

Choose a reason for hiding this comment

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

Putting this in a let block has caused the rest of the file to be indented. Maybe review with whitespace changes hidden...

Or we could come up with a better name for the helper and expose it publicly (removing the let-block).

Copy link
Member Author

Choose a reason for hiding this comment

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

That said, almost all lines were touched anyway, so the extra indent probably isn't a big deal 🤔

Copy link
Member

@traxys traxys left a comment

Choose a reason for hiding this comment

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

LGTM

@MattSturgeon MattSturgeon merged commit f7e009d into nix-community:main Jun 2, 2024
55 checks passed
@MattSturgeon MattSturgeon deleted the default_null_opts_prime branch June 2, 2024 15:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants