nix-community / nixvim

Configure Neovim with Nix! [maintainer=@GaetanLepage, @traxys, @mattsturgeon]
https://nix-community.github.io/nixvim
MIT License
1.44k stars 220 forks source link

[BUG] "global prefix" boolean settings are not converted to int #1741

Open henrispriet opened 1 month ago

henrispriet commented 1 month ago
Field Description
Plugin lazygit
Nixpkgs unstable
Home Manager unstable

Description

When plugins.lazygit.settings.use_custom_config_file_path is set to true, lazygit.nvim does not recognize it.

NixVim says this option is a bool and passes it on as such into the generated init.lua, but lazygit.nvim checks if the option is 1 to enable the feature.

Minimal, Reproducible Example (MRE)

programs.nixvim = {
  plugins.lazygit.settings = {
    use_custom_config_file_path = true;
    config_file_path = builtins.toString (pkgs.writeText "custom-lazygit-config.yml" ''
      # any options, e.g.
      gui:
        border: single
      notARepository: quit
    '');
  };
};

Workaround

programs.nixvim = {
  plugins.lazygit.settings = {
-   use_custom_config_file_path = true;
+   use_custom_config_file_path.__raw = 1;
    config_file_path = builtins.toString (pkgs.writeText "custom-lazygit-config.yml" ''
      # any options, e.g.
      gui:
        border: single
      notARepository: quit
    '');
  };
};
MattSturgeon commented 1 month ago

As per the example, mkVimPlugin should convert bool settings options to int. https://github.com/nix-community/nixvim/blob/49452662b7b4dd2467cbac19e0f9820d570d8976/lib/vim-plugin.nix#L57-L62

The example was added by @GaetanLepage in 97fb6d6a

That said, looking at the implementation, I can't see any type conversion being done. It only seems to add the prefix to the option name: https://github.com/nix-community/nixvim/blob/49452662b7b4dd2467cbac19e0f9820d570d8976/lib/vim-plugin.nix#L107

This likely affects a ton of plugins, as many will assume the example is correct and bools are properly converted to int.


I see two solutions to this:

  1. Have mkVimPlugin auto-convert cfg bools
  2. Introduce a mkVimBool option helper, with type oneOf [ true false 1 0 ]

The mkVimPlugin approach could replace nameValuePair with a custom processGlobal (name TBC):

processGlobal = prefix: name: value: {
  name = prefix + name;
  value =
    if isBool value then
      if value then 1 else 0
    else
      value;
};

# Used like:
globals = mapAttrs' (processGlobal globalPrefix) globals; 

The mkVimBool approach could have an apply function, e.g.

apply =
  v:
  if isBool v then
    if v then 1 else 0
  else
    v;

On the other hand, in the spirit of rfc42, maybe we should encourage using the appropriate type, in this case enum [0 1].

We could add temporary support for booleans along with a deprecation warning when they are used?

MattSturgeon commented 1 month ago

I'm dumb, of course vim options can be booleans. Therefore we can't assume nix booleans should be converted to ints.

Instead, the settings-example should be fixed and any affected options should have their type changed to enum [0 1].

IDK if there's a good way to transition affected options without immediately breaking users' configs. I guess any affected users already had (silently) broken configs anyway, so this may not be important.

One way we could do this would be to create a custom type, equivalent to enum [0 1 true false] but have the type's description be "either 0 or 1".

That way we aren't documenting boolean support, but we could still check for boolean definitions and print an appropriate warning.

traxys commented 1 month ago

Yeah for configs that are already broken I don't think it's too bad to introduce a breaking change