treewide: standardize shell integration options

[trueNAHO]

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

[rycee]

Very cool, thanks for the contribution! I've added a few minor comments.

[rycee]

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;

[trueNAHO]

What do you think of the following approach:

From 68f25c5432b9bc04b57ee000b008d8639f4ff35f Mon Sep 17 00:00:00 2001
From: NAHO <90870942+trueNAHO@users.noreply.github.com>
Date: Mon, 27 Jan 2025 16:11:27 +0100
Subject: [PATCH 1/2] lib: shell: mkShellIntegrationOption: make default and
 example static

Make the default and example values static to prevent costly
documentation rebuilds.

This change is non-atomic and breaks dependant code.
---
 modules/lib/shell.nix | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/modules/lib/shell.nix b/modules/lib/shell.nix
index 2614a6fa..8ed6bfed 100644
--- a/modules/lib/shell.nix
+++ b/modules/lib/shell.nix
@@ -3,8 +3,11 @@
 let
   mkShellIntegrationOption = name: default:
     lib.mkEnableOption "${name} integration" // {
-      inherit default;
-      example = !default;
+      defaultText = lib.literalExpression "config.shell.enable${
+          if default == null then name else default
+        }Integration";
+
+      example = false;
     };
 in rec {
   # Produces a Bourne shell like variable export statement.
@@ -15,9 +18,9 @@ in rec {
   # statement for each set entry.
   exportAll = vars: lib.concatStringsSep "\n" (lib.mapAttrsToList export vars);

-  mkBashIntegrationOption = mkShellIntegrationOption "Bash";
-  mkFishIntegrationOption = mkShellIntegrationOption "Fish";
-  mkIonIntegrationOption = mkShellIntegrationOption "Ion";
-  mkNushellIntegrationOption = mkShellIntegrationOption "Nushell";
-  mkZshIntegrationOption = mkShellIntegrationOption "Zsh";
+  mkBashIntegrationOption = mkShellIntegrationOption "Bash" null;
+  mkFishIntegrationOption = mkShellIntegrationOption "Fish" null;
+  mkIonIntegrationOption = mkShellIntegrationOption "Ion" null;
+  mkNushellIntegrationOption = mkShellIntegrationOption "Nushell" null;
+  mkZshIntegrationOption = mkShellIntegrationOption "Zsh" null;
 }
--
2.47.0

Unsure which of the possible (untested) approaches (assuming they work) we should use:

From f32f08d4e269b43d52e8d89577e3798381811a9c Mon Sep 17 00:00:00 2001
From: NAHO <90870942+trueNAHO@users.noreply.github.com>
Date: Mon, 27 Jan 2025 16:14:28 +0100
Subject: [PATCH 2/2] autojump: PoC application of new mkShellIntegrationOption
 pattern

I assume the following pattern:

    config = mkIf cfg.enable {
      ${cfg}.enable<SHELL>Integration =
        config.shell.enable<SHELL>Integration;

      /* ... */
    };

should be preferred over the following technically more correct
alternative:

    config = mkMerge [
      {
        ${cfg}.enable<SHELL>Integration =
          config.shell.enable<SHELL>Integration;
      }

      (mkIf cfg.enable {
        /* ... */
      })
    ]

Maybe the following slightly simpler variant of declaring config twice,
still merges everything properly:

    config = {
      ${cfg}.enable<SHELL>Integration =
        config.shell.enable<SHELL>Integration;
    };

    config = mkIf cfg.enable {
      /* ... */
    };
---
 modules/programs/autojump.nix | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/modules/programs/autojump.nix b/modules/programs/autojump.nix
index 441fe174..abb84821 100644
--- a/modules/programs/autojump.nix
+++ b/modules/programs/autojump.nix
@@ -12,18 +12,18 @@ in {

   options.programs.autojump = {
     enable = mkEnableOption "autojump";
-
-    enableBashIntegration =
-      lib.hm.shell.mkBashIntegrationOption config.shell.enableBashIntegration;
-
-    enableFishIntegration =
-      lib.hm.shell.mkFishIntegrationOption config.shell.enableFishIntegration;
-
-    enableZshIntegration =
-      lib.hm.shell.mkZshIntegrationOption config.shell.enableZshIntegration;
+    enableBashIntegration = lib.hm.shell.mkBashIntegrationOption;
+    enableFishIntegration = lib.hm.shell.mkFishIntegrationOption;
+    enableZshIntegration = lib.hm.shell.mkZshIntegrationOption;
   };

   config = mkIf cfg.enable {
+    ${cfg} = {
+      enableBashIntegration = config.shell.enableBashIntegration;
+      enableFishIntegration = config.shell.enableFishIntegration;
+      enableZshIntegration = config.shell.enableZshIntegration;
+    };
+
     home.packages = [ package ];

     programs.bash.initExtra = mkIf cfg.enableBashIntegration (mkBefore ''
--
2.47.0

[trueNAHO]

Changelog

v1: 80001f1

• /modules/misc/shell.nix: reorder entries and fix typo

v0: 4534c00

[rycee]

@trueNAHO I pushed a commit with my proposal for how it could be done. To my eyes this is a bit cleaner and as far as I can tell there are no dependencies between the manual build and the user's configuration.

One thing I'm wondering is whether it really is necessary to have an enableShellIntegration option at all, I couldn't think of any compelling use-case.

Another thing is whether it perhaps would be good to have these top-level options under home. instead of shell.. I.e., home.enableBashIntegration or even home.enableBashIntegrationGlobally 🙂

[trueNAHO]

I pushed a commit with my proposal for how it could be done. To my eyes this is a bit cleaner

LGTM.

as far as I can tell there are no dependencies between the manual build and the user's configuration.

If defaultText is what is used to build the manual, this should be fine.

One thing I'm wondering is whether it really is necessary to have an enableShellIntegration option at all, I couldn't think of any compelling use-case.

Declaring shell.enableShellIntegration = true ensures that all my enabled shells receive all shell integrations provided by Home Manager.

Another thing is whether it perhaps would be good to have these top-level options under home. instead of shell.. I.e., home.enableBashIntegration or even home.enableBashIntegrationGlobally 🙂

Since the home namespace already seems to be used by some global options like home.enableDebugInfo and home.enableNixpkgsReleaseCheck, we could move the options from shell.* to home.shell.*. To what files should we move the new /modules/lib/shell.nix and /modules/misc/shell.nix content in that case?

I think home.shell.enableBashIntegration is better than home.shell.enableBashIntegrationGlobally considering that it should be somewhat obvious and that the description already explicitly points this out.

[rycee]

My idea with the shell.enableShellIntegration perhaps not being necessary would be that the shell.enableBashIntegration, shell.enableZshIntegration, etc. still exist and simply have true as default value.

The downside would be that if you really want to disable all shell integration then you would have to do so for each of them. I suspect that is such a rare occurrence, though, that the pedagogical advantage of less indirection would be worth it…

[khaneliman]

My idea with the shell.enableShellIntegration perhaps not being necessary would be that the shell.enableBashIntegration, shell.enableZshIntegration, etc. still exist and simply have true as default value.

The downside would be that if you really want to disable all shell integration then you would have to do so for each of them. I suspect that is such a rare occurrence, though, that the pedagogical advantage of less indirection would be worth it…

I like this approach to standardize it treewide but allow overriding disabling. There are some programs that the "integration" auto launches it ie: zellij and user's would want to disable that integration explicitly.

[khaneliman]

Sorry for the noise... was rebasing to resolve conflict and didn't realize you had just pushed something before that.

[rycee]

I probably explained poorly, I meant something like 5bc459a. It would of course still be able to override the shell integration for a specific module, just not change the default value across all shells at once.

[trueNAHO]

I probably explained poorly, I meant something like 5bc459a. It would of course still be able to override the shell integration for a specific module, just not change the default value across all shells at once.

Actually, this is the whole purpose of the enableShellIntegration variable, as visualized in the following dependency chain (ALL_SHELLS -> EACH_SHELL -> EACH_PROGRAM_EACH_SHELL):

                          enableBashIntegration       programs.gpg-agent.enableBashIntegration
                          enableFishIntegration       programs.gpg-agent.enableFishIntegration
enableShellIntegration -> enableIonIntegration     -> programs.gpg-agent.enableIonIntegration
                          enableNushellIntegration    programs.gpg-agent.enableNushellIntegration
                          enableZshIntegration        programs.gpg-agent.enableZshIntegration

Removing the enableShellIntegration option simply removes the following convenience:

Declaring shell.enableShellIntegration = true ensures that all my enabled shells receive all shell integrations provided by Home Manager.

-- #6358 (comment)

[khaneliman]

I probably explained poorly, I meant something like 5bc459a. It would of course still be able to override the shell integration for a specific module, just not change the default value across all shells at once.

Actually, this is the whole purpose of the enableShellIntegration variable, as visualized in the following dependency chain (ALL_SHELLS -> EACH_SHELL -> EACH_PROGRAM_EACH_SHELL):

                          enableBashIntegration       programs.gpg-agent.enableBashIntegration
                          enableFishIntegration       programs.gpg-agent.enableFishIntegration
enableShellIntegration -> enableIonIntegration     -> programs.gpg-agent.enableIonIntegration
                          enableNushellIntegration    programs.gpg-agent.enableNushellIntegration
                          enableZshIntegration        programs.gpg-agent.enableZshIntegration

Removing the enableShellIntegration option simply removes the following convenience:

Declaring shell.enableShellIntegration = true ensures that all my enabled shells receive all shell integrations provided by Home Manager.
-- #6358 (comment)

I agree, this feels powerful and flexible. If the options default to false, a user can choose to keep toggling things per application module, toggle a specific shell 's integrations treewide or all shell integrations treewide.