-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
nushell: Structured settings #6184
base: master
Are you sure you want to change the base?
nushell: Structured settings #6184
Conversation
5d96d63
to
bd98b3f
Compare
modules/programs/nushell.nix
Outdated
(let | ||
flattenSettings = settings: | ||
let | ||
unravel = prefixes: value: | ||
if (lib.isAttrs value && !isNushellInline value) then | ||
lib.flatten | ||
(map (key: unravel (prefixes ++ [ key ]) value.${key}) | ||
(builtins.attrNames value)) | ||
else | ||
lib.nameValuePair (lib.concatStringsSep "." prefixes) value; | ||
in lib.listToAttrs (unravel [ ] settings); | ||
|
||
flattenedSettings = flattenSettings cfg.settings; | ||
in lib.mkIf (cfg.settings != { }) (lib.concatLines (lib.mapAttrsToList | ||
(key: value: "$env.config.${key} = ${toNushell { } value}") | ||
flattenedSettings))) |
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.
An alternative implementation that I personally find slightly more readable and is potentially a bit more efficient since it avoids the intermediate attribute set (although I imagine you'll need many settings for it to be noticeable 🙂).
I haven't tested it beyond the test case and feel free to reject the suggestion if you prefer the original version:
(let | |
flattenSettings = settings: | |
let | |
unravel = prefixes: value: | |
if (lib.isAttrs value && !isNushellInline value) then | |
lib.flatten | |
(map (key: unravel (prefixes ++ [ key ]) value.${key}) | |
(builtins.attrNames value)) | |
else | |
lib.nameValuePair (lib.concatStringsSep "." prefixes) value; | |
in lib.listToAttrs (unravel [ ] settings); | |
flattenedSettings = flattenSettings cfg.settings; | |
in lib.mkIf (cfg.settings != { }) (lib.concatLines (lib.mapAttrsToList | |
(key: value: "$env.config.${key} = ${toNushell { } value}") | |
flattenedSettings))) | |
(let | |
flattenSettings = let | |
joinDot = a: b: "${if a == "" then "" else "${a}."}${b}"; | |
unravel = prefix: value: | |
if lib.isAttrs value && !isNushellInline value then | |
lib.concatMap (key: unravel (joinDot prefix key) value.${key}) | |
(builtins.attrNames value) | |
else | |
[ (lib.nameValuePair prefix value) ]; | |
in unravel ""; | |
mkLine = { name, value }: '' | |
$env.config.${name} = ${toNushell { } value} | |
''; | |
settingsLines = | |
lib.concatMapStrings mkLine (flattenSettings cfg.settings); | |
in lib.mkIf (cfg.settings != { }) settingsLines) | |
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.
Ooh thanks! I prefer your implementation, it's way easier to understand 😄
bd98b3f
to
16aa273
Compare
bump @rycee :3 |
@nyabinary do you think anything in this PR needs to change due to the release? The only thing I can think of right now is changing some examples to use Also, according to the new docs using the |
Would be better to just do it in this PR tbh, maybe rename it to |
16aa273
to
1e2e435
Compare
- Remove 'with lib' - More idiomatic lib calls - Update config file examples with current best practices
1e2e435
to
ea0b5ec
Compare
@nyabinary it was a simple-enough change that I included it in this PR to avoid the extra overhead ✨ |
|| aliasesStr != "" || cfg.settings != { }; | ||
|
||
aliasesStr = lib.concatLines | ||
(lib.mapAttrsToList (k: v: "alias ${k} = ${v}") cfg.shellAliases); |
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.
(lib.mapAttrsToList (k: v: "alias ${k} = ${v}") cfg.shellAliases); | |
(lib.mapAttrsToList (k: v: "alias '${k}' = ${v}") cfg.shellAliases); |
This should make it possible to define aliases with spaces. I figured it might make sense to include in this PR, if that's alright.
Description
Closes #5527 (but it should be fixed on new nushell versions regardless of this PR if
programs.nushell.configFile
is set in an adequate way)default_env.nu
/default_config.nu
before userenv.nu
/config.nu
nushell/nushell#14249 nushell loads the default config before the user config, removing the need to explicitly set it.Hence, this PR reintroduces the
programs.nushell.settings
option. It then flattens the config and assigns each value to$env.config
. The flattening step is done to avoid overwriting previously defined config values (otherwise,completions.quick = true
would become$env.config.completions = { quick: true }
, deleting all othercompletions
config).becomes
One thing I'm wondering is if the code should have a special cases for lists/records, and append/merge them instead of assigning them. This could be implemented on a followup PR if we see the need 🤔
Checklist
Change is backwards compatible.
Code formatted with
./format
.Code tested through
nix-shell --pure tests -A run.all
ornix develop --ignore-environment .#all
using Flakes.Test cases updated/added. See example.
Commit messages are formatted like
See CONTRIBUTING for more information and recent commit messages for examples.
Maintainer CC
@Philipp-M @aidalgol