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

stylix: improve Bash expression #318

Merged

Conversation

trueNAHO
Copy link
Collaborator

@trueNAHO trueNAHO commented Apr 1, 2024

Addresses: #317 (comment)

@danth danth added the technical debt Things which should be cleaned up or improved label Apr 1, 2024
@danth
Copy link
Owner

danth commented Apr 1, 2024

I suspect there are other places throughout various modules also in need of attention.

There are a few functions in lib which could be helpful when substituting data from Nix into a Bash script:

  • lib.escapeShellArg
  • lib.escapeShellArgs
  • lib.toShellVar
  • lib.toShellVars
  • lib.toGNUCommandLineShell

See their definitions here and here for usage instructions.

Using pkgs.writeShellApplication rather than pkgs.writeShellScript or pkgs.writeShellScriptBin is also preferable, since it runs the generated script through ShellCheck.

@danth
Copy link
Owner

danth commented Apr 1, 2024

Wait - this is a pull request, not an issue. Oh well.

@danth danth removed the technical debt Things which should be cleaned up or improved label Apr 1, 2024
Copy link
Owner

@danth danth left a comment

Choose a reason for hiding this comment

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

lib.escapeShellArg is preferable since it handles the case where the string to be substituted contains a quotation mark.

@trueNAHO
Copy link
Collaborator Author

trueNAHO commented Apr 2, 2024

lib.escapeShellArg is preferable since it handles the case where the string to be substituted contains a quotation mark.

NixOS/nixpkgs#16973 resolved my confusion around ${lib.escapeShellArg pkgs.hello.pname} being preferred over "${pkgs.hello.pname}" or '${pkgs.hello.pname}':

@trueNAHO trueNAHO mentioned this pull request Apr 2, 2024
2 tasks
@trueNAHO trueNAHO requested a review from danth April 2, 2024 14:16
@trueNAHO
Copy link
Collaborator Author

trueNAHO commented Apr 2, 2024

Potentially closes: #324.

@trueNAHO trueNAHO mentioned this pull request Apr 3, 2024
2 tasks
@danth
Copy link
Owner

danth commented Apr 3, 2024

I see escapeShellArg being necessary for cfg.image since the file path is user-defined and could potentially contain any character.

cfg.polarity is enforced by the type system to only ever be one of light, dark or either, so we don't actually need quotation marks at all in that case since it will always be a single word.

@trueNAHO
Copy link
Collaborator Author

trueNAHO commented Apr 4, 2024

I see escapeShellArg being necessary for cfg.image since the file path is user-defined and could potentially contain any character.

Agreed. Unfortunately, despite my understanding of lib.escapeShellArg (#318 (comment)), I do not understand why f9b9bc7 resolved the mentioned issue. Do you know what the proper fix should be?

cfg.polarity is enforced by the type system to only ever be one of light, dark or either, so we don't actually need quotation marks at all in that case since it will always be a single word.

True. However, my Bash heart aches seeing a variable not being double quoted, as the cfg.polarity values may expand to multiple words in the future.

@trueNAHO trueNAHO force-pushed the refactor-stylix-palette-improve-bash-expression branch from cb94428 to 57f8061 Compare April 4, 2024 09:38
@danth
Copy link
Owner

danth commented Apr 4, 2024

lib.escapeShellArg arg uses toString arg, which behaves slightly differently to the ${arg} we were using previously.

https://github.com/NixOS/nixpkgs/blob/4f7b6bb6943837310c439a6d110f871da255e85e/lib/strings.nix#L454

Perhaps lib.escapeShellArg "${arg}" would get the proper result.

@danth danth merged commit 406f793 into danth:master Apr 11, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

stylix: hotfix stylix/palette.nix Wallpaper withBinaryFile does not exist (No such file or directory)
2 participants