Skip to content

Commit

Permalink
lib/types: Handle submodules for type "either"
Browse files Browse the repository at this point in the history
So far the "either" type only handled "flat" types, so you couldn't do
something like:

type = either int (submodule {
  options = ...;
});

Not only caused this the submodule's options not being checked but also
not show up in the documentation.

This was something we stumbled on with #13916.

Signed-off-by: aszlig <aszlig@redmoonstudios.org>
Cc: @edolstra
  • Loading branch information
aszlig committed Mar 19, 2016
1 parent 3c060b9 commit 0f0805b
Showing 1 changed file with 19 additions and 1 deletion.
20 changes: 19 additions & 1 deletion lib/types.nix
Original file line number Diff line number Diff line change
Expand Up @@ -246,7 +246,25 @@ rec {
either = t1: t2: mkOptionType {
name = "${t1.name} or ${t2.name}";
check = x: t1.check x || t2.check x;
merge = mergeOneOption;
merge = loc: defs:
if all t1.check (getValues defs) then t1.merge loc defs
else if all t2.check (getValues defs) then t2.merge loc defs
else throw ( "The option `${showOption loc}' has conflicting"
+ " definitions for type either, in"
+ " ${showFiles (getFiles defs)}.");

getSubOptions = prefix: t1.getSubOptions prefix
// t2.getSubOptions prefix;

getSubModules = concatLists (remove null [
t1.getSubModules
t2.getSubModules
]);

substSubModules = m: let
maybeDef = def: r: if r == null then def else r;
in either (maybeDef t1 (t1.substSubModules m))
(maybeDef t2 (t2.substSubModules m));
};

# Obsolete alternative to configOf. It takes its option
Expand Down

4 comments on commit 0f0805b

@nbp
Copy link
Member

@nbp nbp commented on 0f0805b Mar 19, 2016

Choose a reason for hiding this comment

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

@aszlig

This patch adds ambiguous behaviour in the way options are processed. One simple value can change the way all other values are interpreted without any warning.

 foo = {
   value = 42;
 };

from one module can make the change the semantic of value if it is defined in both submodules.

If we are going to do so, I think the merge function should ensure that the set of options are completely disjoint.

In the mean time, I do not think this is a good idea, and suggest we should not accept this patch.
Also, why did this patch went into master without requesting a review?

@aszlig
Copy link
Member Author

@aszlig aszlig commented on 0f0805b Mar 19, 2016

Choose a reason for hiding this comment

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

@nbp: You mean if you use something like either submodule1 submodule2?

Other than that, I've tested the change by building the manual and checking against a few module declarations, but I'll revert and submit a PR for review.

@aszlig
Copy link
Member Author

@aszlig aszlig commented on 0f0805b Mar 19, 2016

Choose a reason for hiding this comment

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

Also the reason I've pushed this directly to master is because we currently do not have option declarations with either ... submodule.

@aszlig
Copy link
Member Author

@aszlig aszlig commented on 0f0805b Mar 19, 2016

Choose a reason for hiding this comment

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

@nbp: See #14053.

Please sign in to comment.