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

treewide: standardize shell integration options #6358

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

trueNAHO
Copy link
Contributor

@trueNAHO trueNAHO commented Jan 25, 2025

Description

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

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

    {component}: {description}
    
    {long description}
    

    See CONTRIBUTING for more information and recent commit messages for examples.

  • If this PR adds a new module

    • Added myself as module maintainer. See example.

Maintainer CC

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
@trueNAHO trueNAHO marked this pull request as draft January 25, 2025 22:39
@trueNAHO trueNAHO marked this pull request as ready for review January 25, 2025 22:40
@trueNAHO trueNAHO mentioned this pull request Jan 25, 2025
6 tasks
Copy link
Member

@rycee rycee left a 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;
Copy link
Member

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

Suggested change
example = !default;
defaultText = literalExpression "config.shell.enableShellIntegration";
example = false;

Comment on lines +10 to +25
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;
Copy link
Member

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 🙂):

Suggested change
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;
Copy link
Member

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.

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.

2 participants