danth / stylix

System-wide colorscheming and typography for NixOS
https://stylix.danth.me/
MIT License
906 stars 105 forks source link

stylix: improve Bash expression #318

Closed trueNAHO closed 2 months ago

trueNAHO commented 3 months ago

Addresses: https://github.com/danth/stylix/pull/317#discussion_r1546389604

danth commented 3 months ago

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:

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 commented 3 months ago

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

trueNAHO commented 3 months ago

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

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

trueNAHO commented 3 months ago

Potentially closes: https://github.com/danth/stylix/issues/324.

danth commented 3 months ago

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 commented 3 months ago

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 (https://github.com/danth/stylix/pull/318#issuecomment-2031808487), I do not understand why https://github.com/danth/stylix/commit/f9b9bc7c8e69942cd2583a3309f86fc5260f1275 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.

danth commented 3 months ago

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.