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/strings/toShellVars: handle derivations as strings #171946

Merged
merged 1 commit into from May 12, 2022

Conversation

ncfavier
Copy link
Member

@ncfavier ncfavier commented May 7, 2022

No description provided.

Copy link
Member

@roberth roberth left a comment

Choose a reason for hiding this comment

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

It's a good change. Glad that you catch this while the function is young; saves some concerns.
Just need to fill in some details and we're good.

@@ -356,7 +356,7 @@ rec {
*/
toShellVar = name: value:
lib.throwIfNot (isValidPosixName name) "toShellVar: ${name} is not a valid shell variable name" (
if isAttrs value then
if isAttrs value && ! isCoercibleToString value then
Copy link
Member

@roberth roberth May 7, 2022

Choose a reason for hiding this comment

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

Needs doc update, like

-     Supported value types are strings (mapped to regular variables), lists of strings
+     Supported value types are string (mapped to a regular variable), string-coercible attrset
+     (also mapped to a regular variable), list of string

EDIT: use singular to avoid ambiguity.

Copy link
Member

Choose a reason for hiding this comment

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

Did you change it? The current text is too implicit.

It's also better to use singular where possible, as it avoids ambiguity.

Copy link
Member Author

@ncfavier ncfavier May 7, 2022

Choose a reason for hiding this comment

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

I did change the next sentence. I think it's just the right amount of implicit: the intent is that strings are mapped to regular variables, and, as a side-note, things that should obviously be treated as strings are treated as strings. Mentioning string-coercible attrsets feels heavy and impedes readability IMO.

👍 for singular

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough.

lib/tests/misc.nix Outdated Show resolved Hide resolved
@roberth roberth merged commit f771d39 into NixOS:master May 12, 2022
@ncfavier ncfavier deleted the toShellVars-derivations branch May 12, 2022 12:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants