-
-
Notifications
You must be signed in to change notification settings - Fork 15.3k
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
Generators default values #37874
Generators default values #37874
Conversation
* properly escape strings * remove one check for booleans * improve error message
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to go from my side once the comments are addressed.
lib/generators.nix
Outdated
"${t} not supported: ${toPretty {} v}"); | ||
in if isInt v then toString v | ||
# we default to not quoting strings | ||
else if isString v then ''${v}'' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If v
is already a string, why antiquote it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was a leftover, because I first thought quoting the string might be a good idea.
doc/functions.xml
Outdated
@@ -221,16 +221,68 @@ | |||
|
|||
<para> | |||
All generators follow a similar call interface: <code>generatorName | |||
configFunctions data</code>, where <literal>configFunctions</literal> is a | |||
set of user-defined functions that format variable parts of the content. | |||
{ configFunctions } data</code>, where <literal>configFunctions</literal> is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really? As I can see this doesn't seem to be the case, it's still like toINI { mkSectionName = ... }
and not toINI { configFunctions.mkSectionName = ... }
, did I miss something? And if yes, then it's a backwards-incompatibility and should have a release notes entry.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, is that confusing? I added the {}
to make it easier to see that it’s an attrset, which is of course not the formally correct way. If it tripped you up, I think I’ll revert that.
So far, `mkValueString` defaulted to `toString`, which is a bad match for most configuration file formats, especially because how booleans are formatted. This also improves error messages for unsupported types. Add a test to codify the formatting.
An example of overriding the `toINI` generator is added, hopefully clarifying the expressiveness of generators.
d5e9343
to
2ef7297
Compare
better default values & documentation; see commit messages.
cc @aszlig