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

plugins/wezterm: init #2597

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

samos667
Copy link
Contributor

@samos667 samos667 commented Dec 1, 2024

This pull request introduces a new Neovim plugin for wezterm and integrates it into the existing plugin system. The changes include adding the wezterm plugin to the molten plugin list, defining the wezterm plugin in its own file, and adding test cases for the new plugin.

New Plugin Addition:

Testing:

fixes #2596

@samos667 samos667 force-pushed the init-wezterm branch 3 times, most recently from a49ca07 to 191626d Compare December 1, 2024 19:32
@samos667 samos667 force-pushed the init-wezterm branch 2 times, most recently from 5cfbfba to dbd8645 Compare December 5, 2024 21:29
@khaneliman
Copy link
Contributor

@samos667 We updated flake lock today so you should be able to rebase this, now.

@samos667
Copy link
Contributor Author

samos667 commented Dec 7, 2024

error: attribute 'samos667' missing

Sadge :'(

@khaneliman
Copy link
Contributor

khaneliman commented Dec 7, 2024

error: attribute 'samos667' missing

Sadge :'(

If you're not in nixpkgs maintainers, you can add yourself to our maintainers file in another commit.

@MattSturgeon
Copy link
Member

We also have a test failure:

defaults> ERROR: Error detected while processing /nix/store/XXX-init.lua:
defaults> Wezterm failed to find 'wezterm' executable
defaults> Wezterm failed to find 'wezterm' executable

Perhaps the nixpkgs package doesn't have wezterm marked as a dependency?

@MattSturgeon MattSturgeon force-pushed the init-wezterm branch 2 times, most recently from 11c4a25 to e1c0570 Compare December 9, 2024 17:41
@MattSturgeon
Copy link
Member

The following patch causes the test to pass, however I believe this should be fixed upstream in nixpkgs.

diff --git a/plugins/by-name/wezterm/default.nix b/plugins/by-name/wezterm/default.nix
index aecb332e..e95b36a4 100644
--- a/plugins/by-name/wezterm/default.nix
+++ b/plugins/by-name/wezterm/default.nix
@@ -1,4 +1,4 @@
-{ lib, ... }:
+{ lib, pkgs, ... }:
 let
   inherit (lib.nixvim) defaultNullOpts;
 in
@@ -9,6 +9,10 @@ lib.nixvim.neovim-plugin.mkNeovimPlugin {
 
   maintainers = [ lib.maintainers.samos667 ];
 
+  extraPackages = [
+    pkgs.wezterm
+  ];
+
   settingsOptions = {
     create_commands = defaultNullOpts.mkBool true ''
       Whether to create plugin commands.

Or if not*, then we should have a nullable weztermPackage option.

weztermPackage = lib.mkPackageOption pkgs "wezterm" {
  nullable = true;
  # maybe also `default = null` and `example = [ "wezterm" ]`?
};

* Perhaps nixpkgs decide that bundling the dep leads to too many potential issues, such as if users have an overridden/custom wezterm installed to their PATH?

@GaetanLepage
Copy link
Member

The following patch causes the test to pass, however I believe this should be fixed upstream in nixpkgs.

diff --git a/plugins/by-name/wezterm/default.nix b/plugins/by-name/wezterm/default.nix
index aecb332e..e95b36a4 100644
--- a/plugins/by-name/wezterm/default.nix
+++ b/plugins/by-name/wezterm/default.nix
@@ -1,4 +1,4 @@
-{ lib, ... }:
+{ lib, pkgs, ... }:
 let
   inherit (lib.nixvim) defaultNullOpts;
 in
@@ -9,6 +9,10 @@ lib.nixvim.neovim-plugin.mkNeovimPlugin {
 
   maintainers = [ lib.maintainers.samos667 ];
 
+  extraPackages = [
+    pkgs.wezterm
+  ];
+
   settingsOptions = {
     create_commands = defaultNullOpts.mkBool true ''
       Whether to create plugin commands.

This is still something we have not clearly agreed on in nixpkgs...
I think that the right approach here is having the suggested weztermPackage option.

Comment on lines +17 to +21
weztermPackage = lib.mkPackageOption pkgs "wezterm" {
nullable = true;
default = null;
example = [ "wezterm" ];
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the best default here? The package or null?

I'm assuming most users who enable this plugin will already have wezterm installed 🤔

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Usually we set the package.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would achieve that:

Suggested change
weztermPackage = lib.mkPackageOption pkgs "wezterm" {
nullable = true;
default = null;
example = [ "wezterm" ];
};
weztermPackage = lib.mkPackageOption pkgs "wezterm" {
nullable = true;
};

Is there any reason not to do that here?

@MattSturgeon
Copy link
Member

@samos667 I've pushed up a couple tweaks. Let us know if you're happy and we should be good to approve & merge.

]
''
How images are displayed.

If set to `"wezterm"`, `plugins.wezterm.enable` must be true.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not adding a warning to check this ? This is usually what we do.

@samos667
Copy link
Contributor Author

Seems good to me !

@MattSturgeon
Copy link
Member

I believe @GaetanLepage is hoping you'll change the following before we approve/merge:

  1. Introduce a warning for plugins/wezterm: init #2597 (comment)
  2. Change the weztermPackage option to no longer default to null plugins/wezterm: init #2597 (comment)

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.

[PLUGIN REQUEST] wezterm.nvim
4 participants