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

Jump to bottom

lib/types: Add oneOf, extension of either to a list of types #65728

Merged
merged 2 commits into from Aug 13, 2019
Merged

lib/types: Add oneOf, extension of either to a list of types #65728

merged 2 commits into from Aug 13, 2019

Conversation

infinisil
Copy link
Member

@infinisil infinisil commented Aug 1, 2019 •
edited

Motivation for this change

Oftentimes a type such as either str (either bool (either int float)) is needed, which is annoying to write. This change allows you to use oneOf [ str bool int float ] instead. This will be useful for the implementation of NixOS/rfcs#42

Ping @Profpatsch @rycee @danbst

Things done
  • Added tests and ran them successfully
  • Added documentation and built it successfully

@infinisil
Copy link
Member Author

As per @grahamc's suggestion on IRC, I renamed it from eithers to oneOf

@infinisil infinisil changed the title lib/types: Add eithers, extension of either to a list of types lib/types: Add oneOf, extension of either to a list of types Aug 1, 2019
@infinisil
Copy link
Member Author

infinisil commented Aug 1, 2019 •
edited

It's not really an ADT though, https://github.com/shlevy/nix-adt/ would be more like it (and might be nice to reserve adt for that something like that). This is more like an untagged union, because you e.g. couldn't distinguish between listOf str and listOf int when the value is [].

Profpatsch
Profpatsch requested changes Aug 6, 2019
View reviewed changes
Copy link
Member

@Profpatsch Profpatsch left a comment

Choose a reason for hiding this comment

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

I like it, one small mistake I spotted. The naming is very good.

lib/types.nix Outdated Show resolved Hide resolved
@infinisil
Copy link
Member Author

Pushed foldl -> foldl'

danbst
danbst approved these changes Aug 6, 2019
View reviewed changes
Copy link
Contributor

@danbst danbst left a comment

Choose a reason for hiding this comment

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

I wanted this as well!

@infinisil
Copy link
Member Author

I pushed a commit that replaces all nested either's in NixOS modules with oneOf, there weren't that many

danbst
danbst reviewed Aug 9, 2019
View reviewed changes
nixos/modules/services/security/bitwarden_rs/default.nix
@@ -36,7 +36,7 @@ in {
};

config = mkOption {
type = attrsOf (nullOr (either (either bool int) str));
type = attrsOf (nullOr (oneOf [ bool int str ]));
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
type = attrsOf (nullOr (oneOf [ bool int str ]));
type = attrsOf (oneOf [ (nullOr bool) int str ]);

or oneOf [ null bool int str ]

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think null is a type? And I prefer the nullOr (oneOf ...), because it gives the meaning of "either unset or one of these"

Copy link
Contributor

Choose a reason for hiding this comment

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

I've suggested it so it would be compatible with my another suggestion - #65728 (comment)

But I'm fine with this as well.

danbst
danbst reviewed Aug 9, 2019
View reviewed changes
nixos/modules/system/boot/systemd-unit-options.nix
@@ -226,7 +226,7 @@ in rec {

environment = mkOption {
default = {};
type = with types; attrsOf (nullOr (either str (either path package)));
type = with types; attrsOf (nullOr (oneOf [ str path package ]));
Copy link
Contributor

Choose a reason for hiding this comment

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

see https://github.com/NixOS/nixpkgs/pull/65728/files#r312359971

danbst
danbst reviewed Aug 9, 2019
View reviewed changes
nixos/modules/system/boot/systemd.nix
@@ -520,7 +520,7 @@ in
};

systemd.globalEnvironment = mkOption {
type = with types; attrsOf (nullOr (either str (either path package)));
type = with types; attrsOf (nullOr (oneOf [ str path package ]));
Copy link
Contributor

Choose a reason for hiding this comment

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

see https://github.com/NixOS/nixpkgs/pull/65728/files#r312359971

@danbst
Copy link
Contributor

danbst commented Aug 9, 2019

It also looks like attrsOf x should have an override attrsOf [ x y z ] = attrsOf (oneOf [ x y z ])

@infinisil
Copy link
Member Author

@danbst I feel like attrsOf [ x y z ] isn't very clear, I'd probably interpret it as "attribute set of a list where the first type is x, second y, third z"

@aanderse aanderse merged commit 6f6468b into NixOS:master Aug 13, 2019
@aanderse
Copy link
Member

Thanks @infinisil! 🎉

@infinisil infinisil deleted the types-eithers branch August 13, 2019 15:55
@infinisil infinisil mentioned this pull request Sep 4, 2019
Merged
10 tasks
@infinisil infinisil added the 6.topic: module system About NixOS module system internals label Mar 19, 2020
@infinisil infinisil mentioned this pull request Aug 19, 2020
Closed
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danbst danbst danbst approved these changes

@Profpatsch Profpatsch Profpatsch approved these changes

@edolstra edolstra Awaiting requested review from edolstra

@nbp nbp Awaiting requested review from nbp

Assignees
No one assigned
Labels
6.topic: module system About NixOS module system internals 6.topic: nixos 8.has: documentation 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@infinisil @danbst @aanderse @Profpatsch