nix-community / nixvim

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

[BUG] plugins.leap: safeLabels = [] does not disable auto-jump #1698

Open fin444 opened 3 months ago

fin444 commented 3 months ago
Field Description
Plugin leap
Nixpkgs unstable e8057b6 (June 5)
Home Manager N/A

Description

According to the documentation, setting plugins.leap.safeLabels = [] should disable auto-jump, however it does not.

Minimal, Reproducible Example (MRE)

programs.nixvim = {
  enable = true;
  plugins.leap = {
    enable = true;
    safeLabels = [];
  };
};

Working Example of Auto-Jump Disabled

programs.nixvim = {
  enable = true;
  extraPlugins = [ pkgs.vimPlugins.leap-nvim ];
  extraConfigLua = ''
    require("leap").add_default_mappings()
    require("leap").opts.safe_labels = {}
  '';
};
GaetanLepage commented 3 months ago

Indeed, empty lists and attrs are ignored during the translation to lua. If you want to explicitly have an empty lua table, you can do the following:

setLabels.__empty = null;
MattSturgeon commented 3 months ago

If we ever get to a point where all settings options (inc attrs and lists) are default-null, we might be able to change the behaviour to only omit null attrs?

Maybe we could have a configurable toLuaObject in the meantime where we can toggle behaviours on a per-plugin/per-usage basis? :thinking:


helpers.toLuaObject'
  {
    omitNullAttrs = true;
    omitEmptyAttrs = false;
    multiline = false;
  }
  {
    someNullAttr = null;
    someEmptyAttrsAttr = { };
    someEmptyListAttr = [ ];
  }
=>
''
  { ["someEmptyAttrsAttr"] = { }, ["someEmptyListAttr"] = { } }
''
traxys commented 3 months ago

I think there used to be a reason for empty tables, I remember @GaetanLepage had done some stuff around that some time ago. A good test would be removing the empty attrs from a full config, and seeing how it increases.

GaetanLepage commented 3 months ago

I think there used to be a reason for empty tables, I remember @GaetanLepage had done some stuff around that some time ago. A good test would be removing the empty attrs from a full config, and seeing how it increases.

At some point, we did made the switch to stop ignoring empty attrs. It turned out to be bad. We had to rely on submodules (through helpers.mkCompositeOption) everywhere we needed nested options. We rolled-back this change.

fin444 commented 3 months ago

Indeed, empty lists and attrs are ignored during the translation to lua. If you want to explicitly have an empty lua table, you can do the following:

setLabels.__empty = null;

Unfortunately safeLabels takes a list, not a table, so this is not an option

MattSturgeon commented 3 months ago

In lua, lists and dicts are both tables.

MattSturgeon commented 3 months ago

You can also do safeLabels.__raw = "{ }" to do it explicitly in raw lua if __empty isn't working for some reason

fin444 commented 3 months ago

On the Nix side, safeLabels is a list. So you can't set __raw or __empty

MattSturgeon commented 3 months ago

Can confirm, the type is nullOr (listOf (maybeRaw str)), so neither __raw or __empty can be used.

This is caused by the underlying issue @traxys brought up in an unrelated discussion: defaultNullOpts.mkListOf does not apply maybeRaw to the list itself, it is only applied to list elements (same formkAttrsOf`).

The fact that __empty can't be used with list-type options is also unfortunate. I think the only way we'd get around that is with a custom listOf type, with an extended check function.

fin444 commented 2 months ago

Sorry for taking a while to test this. The changes don't seem to work, auto-jumping still occurs when setting safeLabels.__empty = null; or safeLabels.__raw = "{}"; The below is generated into the config, however. Maybe part of how tbl_deep_extend works?

require("leap").opts = vim.tbl_deep_extend("keep", { safe_labels = {} }, require("leap").opts)
MattSturgeon commented 2 months ago

That sounds likely. The upstream examples all use this pattern:

require('leap').opts[opt_name] = opt_value

I think we either need to do that or use a different "deep extend" mode (maybe force?)

traxys commented 2 months ago

I'd rather follow what upstream recommends