-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
zsh: generated plugin loading cleanup/improvements #7849
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
base: master
Are you sure you want to change the base?
Conversation
11f46cd
to
fd07a18
Compare
Replace individual conditional checks with a single loop over a plugin array. This generates cleaner shell code and follows common bash patterns while maintaining the same functionality. Signed-off-by: Austin Horstman <[email protected]>
Add test assertions to verify the new array-based plugin loading structure generates the expected shell code patterns. Signed-off-by: Austin Horstman <[email protected]>
Replace individual plugin PATH and fpath statements with efficient array-based loops, reducing generated shell code and improving consistency with existing plugin sourcing logic. Signed-off-by: Austin Horstman <[email protected]>
Update test assertions to match the new array and loop structure for plugin directory and completion path setup, replacing checks for individual statements with loop-based pattern verification. Signed-off-by: Austin Horstman <[email protected]>
Use arrays and loops for bindkey statements instead of individual statements, improving consistency with other zsh optimizations and reducing generated shell code. Signed-off-by: Austin Horstman <[email protected]>
Update test expectations to match the new array and loop structure for key bindings instead of checking for individual bindkey statements. Signed-off-by: Austin Horstman <[email protected]>
Replace repetitive setopt/unsetopt conditional statements with efficient array-based loops, reducing generated shell code and improving consistency with other zsh module optimizations. Signed-off-by: Austin Horstman <[email protected]>
Replace individual setopt statements with array-based loops for cfg.setOptions, improving consistency with other zsh optimizations and reducing generated shell code. Signed-off-by: Austin Horstman <[email protected]>
…zation Add shared utility function that formats shell arrays with smart width optimization. Signed-off-by: Austin Horstman <[email protected]>
Improve array formatting to pack multiple items per line based on available width (~76 characters), making better use of terminal space while maintaining readability. Examples: - Before: Each item on separate line - After: Multiple items per line within width limit disabled_opts=( "APPEND_HISTORY" "EXTENDED_HISTORY" "HIST_EXPIRE_DUPS_FIRST" "HIST_FIND_NO_DUPS" "HIST_IGNORE_ALL_DUPS" "HIST_SAVE_NO_DUPS" ) This provides optimal balance of compactness and readability. Signed-off-by: Austin Horstman <[email protected]>
fd07a18
to
2b0da06
Compare
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.
I like the change, I read it commit-by-commit so suggestions might be slightly repetitive.
plugins=( | ||
${lib.concatMapStringsSep "\n " (path: ''"${path}"'') pluginPaths} | ||
) |
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.
We have lib.hm.zsh.define
to do this. Arguably the formatting won't be quite as pretty (no newlines -- this could be fixed in define
itself though)
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.
As a note, I think both toZshValue
and your own version of it fail to account for escaping caracters in a string.
This line should probably read lib.escapeShellArg v
. Your mapped function should probably be lib.escapeShellArg
instead of a naive quote.
) | ||
for plugin in "''${plugins[@]}"; do | ||
[[ -f "${pluginsDir}/$plugin" ]] && source "${pluginsDir}/$plugin" | ||
done |
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.
I think we should unset plugin plugins
after the loop.
plugin_dirs=( | ||
${lib.concatMapStringsSep "\n " (name: ''"${name}"'') pluginNames} | ||
) | ||
for plugin_dir in "''${plugin_dirs[@]}"; do | ||
path+="${pluginsDir}/$plugin_dir" | ||
fpath+="${pluginsDir}/$plugin_dir" | ||
done | ||
${lib.optionalString (completionPaths != [ ]) '' | ||
# Add completion paths to fpath | ||
completion_paths=( | ||
${lib.concatMapStringsSep "\n " (path: ''"${path}"'') completionPaths} | ||
) | ||
for completion_path in "''${completion_paths[@]}"; do | ||
fpath+="${pluginsDir}/$completion_path" | ||
done | ||
''} |
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.
Same suggestions.
# Bind search up keys | ||
search_up_keys=(${lib.concatStringsSep " " (map (key: ''"${key}"'') upKeys)}) | ||
for key in "''${search_up_keys[@]}"; do | ||
bindkey "$key" history-substring-search-up | ||
done | ||
# Bind search down keys | ||
search_down_keys=(${lib.concatStringsSep " " (map (key: ''"${key}"'') downKeys)}) | ||
for key in "''${search_down_keys[@]}"; do | ||
bindkey "$key" history-substring-search-down | ||
done |
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.
Same suggestions.
lib.filter (s: s != "") [ | ||
(lib.optionalString (enabledOpts != { }) '' | ||
# Enabled history options | ||
enabled_opts=(${lib.concatStringsSep " " (lib.mapAttrsToList (name: _: ''"${name}"'') enabledOpts)}) | ||
for opt in "''${enabled_opts[@]}"; do | ||
setopt "$opt" | ||
done'') | ||
(lib.optionalString (disabledOpts != { }) '' | ||
# Disabled history options | ||
disabled_opts=(${ | ||
lib.concatStringsSep " " (lib.mapAttrsToList (name: _: ''"${name}"'') disabledOpts) | ||
}) | ||
for opt in "''${disabled_opts[@]}"; do | ||
unsetopt "$opt" | ||
done'') | ||
] |
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.
Same suggestions.
# Set shell options | ||
set_opts=(${concatStringsSep " " (map (opt: ''"${opt}"'') cfg.setOptions)}) | ||
for opt in "''${set_opts[@]}"; do | ||
setopt "$opt" | ||
done |
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.
Same suggestions.
Also: should the history options be deprecated to use cfg.setOptions
instead? That'd be a big change so definitely not suggesting it for this MR, just floating the idea.
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.
I've been thinking about that, too. I mentioned the idea #7333 (comment) just wasn't 100% sure, yet.
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.
We could use this in lib.hm.zsh.define
directly, I guess.
mkOrder 950 '' | ||
# Set shell options | ||
set_opts=(${concatStringsSep " " (map (opt: ''"${opt}"'') cfg.setOptions)}) | ||
set_opts=(${lib.hm.shell.formatShellArray cfg.setOptions}) |
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.
... Because I think this is slightly weird: formatShellArray
does not define an array (i.e: I have to call it inside the (
/)
).
Description
Just doing some loops for adding to path and sourcing to clean up the generated code.
Plugin path:
->
Plugin sourcing:
->
Hist:
->
Checklist
Change is backwards compatible.
Code formatted with
nix fmt
ornix-shell -p treefmt nixfmt deadnix keep-sorted --run treefmt
.Code tested through
nix run .#tests -- test-all
ornix-shell --pure tests -A run.all
.Test cases updated/added. See example.
Commit messages are formatted like
See CONTRIBUTING for more information and recent commit messages for examples.
If this PR adds a new module
If this PR adds an exciting new feature or contains a breaking change.