-
-
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
treewide: standardize shell integration options #6358
base: master
Are you sure you want to change the base?
treewide: standardize shell integration options #6358
Conversation
Standardize all 'programs.<PROGRAM>.enable<SHELL>Integration' options with the following new functions: - lib.hm.shell.mkBashIntegrationOption - lib.hm.shell.mkFishIntegrationOption - lib.hm.shell.mkIonIntegrationOption - lib.hm.shell.mkNushellIntegrationOption - lib.hm.shell.mkZshIntegrationOption These functions should default to their corresponding global option: - shell.enableBashIntegration - shell.enableFishIntegration - shell.enableIonIntegration - shell.enableNushellIntegration - shell.enableZshIntegration All these global options default to the 'shell.enableShellIntegration' value. This hierarchy standardizes the shell integration and increases end-user flexibility. BREAKING CHANGE: This modifies the following inconsistent default values from 'false' to 'true': - programs.zellij.enableBashIntegration - programs.zellij.enableFishIntegration - programs.zellij.enableZshIntegration
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.
Very cool, thanks for the contribution! I've added a few minor comments.
mkShellIntegrationOption = name: default: | ||
lib.mkEnableOption "${name} integration" // { | ||
inherit default; | ||
example = !default; |
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.
Unfortunately cannot allow default
or example
to be affected by the configuration since it would in turn affect the documentation build. Can resolve it by adding a defaultText
and make the example hard-coded. Something like this (untested) perhaps
example = !default; | |
defaultText = literalExpression "config.shell.enableShellIntegration"; | |
example = false; |
default = true; | ||
|
||
description = '' | ||
Whether to enable shell integration for all supported shells. | ||
|
||
Individual shell integrations can be overriden with their respective | ||
`shell.enable<SHELL>Integration` option. For example, the following | ||
declaration globally disables shell integration for Bash: | ||
|
||
```nix | ||
config.shell.enableBashIntegration = false; | ||
``` | ||
''; | ||
|
||
example = false; | ||
type = lib.types.bool; |
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.
Can sort the field in the more idiomatic way (also fix the overridden typo 🙂):
default = true; | |
description = '' | |
Whether to enable shell integration for all supported shells. | |
Individual shell integrations can be overriden with their respective | |
`shell.enable<SHELL>Integration` option. For example, the following | |
declaration globally disables shell integration for Bash: | |
```nix | |
config.shell.enableBashIntegration = false; | |
``` | |
''; | |
example = false; | |
type = lib.types.bool; | |
type = lib.types.bool; | |
default = true; | |
example = false; | |
description = '' | |
Whether to enable shell integration for all supported shells. | |
Individual shell integrations can be overridden with their respective | |
`shell.enable<SHELL>Integration` option. For example, the following | |
declaration globally disables shell integration for Bash: | |
```nix | |
config.shell.enableBashIntegration = false; | |
``` | |
''; |
type = lib.types.bool; | ||
}; | ||
|
||
enableZshIntegration = lib.hm.shell.mkZshIntegrationOption enable; |
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 you can put this together with the other enable<shell>Integration
options above.
Description
Checklist
Change is backwards compatible.
Code formatted with
./format
.Code tested through
nix-shell --pure tests -A run.all
or
nix build --reference-lock-file flake.lock ./tests#test-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.
If this PR adds a new module
Maintainer CC