-
-
Notifications
You must be signed in to change notification settings - Fork 14k
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.mkOptions: allow arbitrary option metadata #341199
base: master
Are you sure you want to change the base?
Conversation
008bb68
to
45b1c5f
Compare
I think this is a good direction.
I'm a little concerned with the UX here because the attribute names won't be checked, or the values for that matter. For it to remain extensible, the type checking would have to added as part of Or would could remove the I think we'll also want to propagate the type information from a module evaluation to all the contained types. We already propagate context like the option prefix, so this would have to follow that pattern. I don't know if this makes the evaluation significantly more expensive. I guess this is the tricky part then. |
I thought of some freeform I'm not sure what would happen in
In the first In the |
I think it makes more sense to allow any arbitrary meta values to be passed in, since
I think |
@roberth What is missing until we can/want to decide if we want to merge this. So should we initially forward the bespoke attributes (description, defaultText, ...) ? Until we actually use them instead of the old ones. Or whats the plan for moving on with this idea? |
It would be up to a few of the many return sub-values of I'm ok to start free-form, if you think it's ok to introduce
I'm not quite certain because I don't have a complete picture for how the migration would go.
So then that means the main code may be ok, but I do have a question. How do you plan to use this field? It doesn't seem that it's propagated to the Could you add a test case showing how you'd like to use this? We have tests in Could you add documentation in |
That makes sense, however I think an additional goal can be to allow module maintainers and/or out-of-tree users to add additional (arbitrary) custom metadata to modules
Sure, it makes sense that well-known attr names would be type-checked where they are used. Such as asserting that
I think that's inevitable, if we want |
Okay so I have two motivations for doing this:
My concrete scenario: I have a set of predefined frontend components (input fields, field arrays, ...). I want the author of a module to define options with added information for rendering and appearance. JobWhenToRun = lib.mkOption {
type = lib.types.str;
meta.renderer = ''DateTime"
meta.minDate = "now"
....
}; |
That is not at all the discussion. The discussion is whether they should be checked; we've already settled that the declarations won't be exclusively in Nixpkgs. |
Sounds like we're all on the same page 🙂 I hadn't considered a If that is the eventual goal, perhaps it'd be best to have it in place from the start, to avoid any downstream users extending |
lib.modules.evalOptionValue is still available
1138bb4
to
2eda2ad
Compare
Came up with an implementation. Could use a few more tests for corner cases and other things we care about, and docs of course. For now I'm curious about the performance impact and whether anything needs to be added to make this practical. I think inheriting meta attributes into submodules could be nice, but that requires changes to the merge function; same for |
Thanks for doing the hard work. I'd like to try to improve the error messages in this case. options.foo = lib.mkOption {
type = types.string;
};
=>
error: The option `_modules.optionMeta.required' was accessed but has no value defined. Try setting the option. This is a little bit confusing because the option is missing an attribute to mkOption. A user would never set I tried various combinations of malformed meta options. Most of them produce confusing error messages. Although they might be technically correct. I am not sure how to fix this, since Maybe we need to fix this in a follow up PR. I thought to make evalModules accept additional error context. That can be printed for giving the error message more meaning. Maybe this could improve the overall error message situation, which is one of the weak points with the module system. |
Also what is more performant?
Because patterns are extendable with new arguments without breaking backwards compatibility. |
… prefix This feels like cheating, but it works out much better for the error messages: nix-repl> :p (evalModules { modules = [ { options.foo = mkOption { }; options._module.optionMeta.wonk = mkOption { }; } ]; }).options.foo.meta error: The option `options.foo.meta.wonk' was accessed but has no value defined. Try setting the option.
Nix has optimizations for the positional style, so I would expect I've changed the prefix to contain the option path. Feels like cheating, but it results in much better errors: nix-repl> :p (evalModules { modules = [ {
options.foo = mkOption { };
options._module.optionMeta.wonk = mkOption { };
} ]; }).options.foo.meta
error: The option `options.foo.meta.wonk' was accessed but has no value defined. Try setting the option. Other error improvements may have to go into follow-ups, but I'm curious what they are and whether this solves a large part of it. |
b350a28
to
67ab16a
Compare
Description of changes
Allows adding arbitrary metadata to options.
@roberth @infinisil I am not sure if this is a good idea. But I could need it to build a nice user interface around the modules.
Examples
Justification for the examples:
(Especially when trying to build a more high level user interface)
-_* \/^
) or just where the name of the option might not make sense for a certain user anymore. I.e.title = "FooBar";
would allow to display the option with a more explanatory label without creating an intermediate option, just for the purpose of rendering.I am not sure if we should add those use-cases to the current keywords, or if we should just allow arbitrary names because i am not sure how the above described scenarios would ideally be represented through new keywords.
Alternatives
instead of explicit
meta ? {}
passthru
instead of meta, maybe it is more clear.Could produce less thunks but also is less safe towards future extensions of mkOption attrs.
Things done
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)Add a 👍 reaction to pull requests you find important.