Skip to content

programs.neovim: Add initExtra option (2)#2391

Closed
viniciusmuller wants to merge 1 commit intonix-community:masterfrom
viniciusmuller:feature-neovim-initextra
Closed

programs.neovim: Add initExtra option (2)#2391
viniciusmuller wants to merge 1 commit intonix-community:masterfrom
viniciusmuller:feature-neovim-initextra

Conversation

@viniciusmuller
Copy link
Copy Markdown

Description

The last PR was truly messy (#2244) so I've closed it and I'm opening a new one now.

Checklist

  • Change is backwards compatible.

  • Code formatted with ./format.

  • Code tested through nix-shell --pure tests -A run.all.

  • 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.

    • Added myself and the module files to .github/CODEOWNERS.

ping @berbiche

@viniciusmuller viniciusmuller requested a review from rycee as a code owner October 9, 2021 16:50
@viniciusmuller viniciusmuller force-pushed the feature-neovim-initextra branch from 2b730bc to b7c8473 Compare October 9, 2021 17:03
Copy link
Copy Markdown
Member

@berbiche berbiche left a comment

Choose a reason for hiding this comment

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

LGTM besides the documentation fix.

What do you think of the changes @teto?

Comment thread modules/programs/neovim.nix Outdated
Comment thread modules/programs/neovim.nix
@teto
Copy link
Copy Markdown
Collaborator

teto commented Oct 9, 2021

With neovim we have people writing their config in lua/fennel/teal; I would like to supporting these approaches in neovim. This means we may have to modify the option aftwards. @berbiche I'll let you handle the reviews.

@viniciusmuller viniciusmuller force-pushed the feature-neovim-initextra branch from b7c8473 to b90d5ed Compare October 10, 2021 16:47
@kclejeune
Copy link
Copy Markdown

kclejeune commented Oct 10, 2021

With neovim we have people writing their config in lua/fennel/teal; I would like to supporting these approaches in neovim. This means we may have to modify the option aftwards. @berbiche I'll let you handle the reviews.

I always assumed that the easiest solution to this in the future would be to add an option specifying the output filetype. We can reuse the existing initExtra option but put it into init.lua or some other file format instead.

This possibly gets a little more complicated if you could have both an init.vim and init.lua for example, but regardless still seems doable without modifying the behavior of this option in particular.

@ratsclub
Copy link
Copy Markdown

I always assumed that the easiest solution to this in the future would be to add an option specifying the output filetype. We can reuse the existing initExtra option but put it into init.lua or some other file format instead.

May we discuss this design decision here? It would be nice as I'm as interested in the initExtra as in using fennel in my configuration. Personally I'm really fond of an option specifying the filetype!

@teto teto added the neovim label Oct 17, 2021
@teto
Copy link
Copy Markdown
Collaborator

teto commented Oct 17, 2021

I would be happy for someone to take on this but we should agree on the design first.
My current take is:

  • to add a freeform "language" option to the plugin submodule (that would default to vim)
  • add a function that collects config with same language and does what it wants with it. By default it would write to init.LANGUAGE

As for the initExtra, someone mentioned putting its initExtra as a fake plugin. Looks like an interesting workaround.

I am scared to add something to complex for users and/or to maintain. At the same time as someone with a complex hybrid lua/viml config (and looking forward to add some fennel), I would still like to make it possible for advanced users.

} // optionalAttrs (cfg.initExtra != "") {
beforePlugins = ''
" Generated by Home Manager
${cfg.initExtra}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Indented here, but not in the test case?

@thiagokokada
Copy link
Copy Markdown
Contributor

I was bitten by this issue after release-21.11. I remap my leader from \ to Space, and since it seems programs.neovim.extraConfig is called later than the plugins my config stopped working (it used to work before).

Could we have this PR merged and backport to release-21.11? It maybe not perfect but at least it solves the issue.

@teto teto mentioned this pull request Nov 30, 2021
5 tasks
@GlancingMind
Copy link
Copy Markdown

I was bitten by this issue after release-21.11. I remap my leader from \ to Space, and since it seems programs.neovim.extraConfig is called later than the plugins my config stopped working (it used to work before).

For now I work around the same Issue by prepanding the first plugins configuration with my nvimrc. E.g.

programs.neovim = {
  ...
  plugins = [
    {
      plugin = pkgs.vimPlugins.gruvbox;
      config = builtins.readFile ./config/nvimrc + builtins.readFile ./config/gruvbox.vim;
    }
  ];
};

@berbiche
Copy link
Copy Markdown
Member

@teto following the merge of #2637, what should be changed here to have this merged?

@teto
Copy link
Copy Markdown
Collaborator

teto commented Feb 27, 2022

I am annoyedby the set nocompatible that creeps in the generated init.vim .
Also now that plugins can specify the language of their config, it would be nice to be able to precise this in option, @selene57 might have a go at this #2716

@berbiche
Copy link
Copy Markdown
Member

@teto I am annoyedby the set nocompatible that creeps in the generated init.vim . Also now that plugins can specify the language of their config, it would be nice to be able to precise this in option, @selene57 might have a go at this #2716

If we drop the the set nocompatible line and keep the Home Manager comment, do you think we could merge this?

@stale
Copy link
Copy Markdown

stale bot commented Jul 17, 2022

Thank you for your contribution! I marked this pull request as stale due to inactivity. Please read the relevant sections below before commenting.

If you are the original author of the PR

  • GitHub sometimes doesn't notify people who commented / reviewed a PR previously when you (force) push commits. If you have addressed the reviews you can officially ask for a review from those who commented to you or anyone else.
  • If it is unfinished but you plan to finish it, please mark it as a draft.
  • If you don't expect to work on it any time soon, please consider closing it with a short comment encouraging someone else to pick up your work.
  • To get things rolling again, rebase the PR against the target branch and address valid comments.

If you are not the original author of the PR

  • If you want to pick up the work on this PR, please create a new PR and indicate that it supercedes and closes this PR.

@stale stale bot added the status: stale label Jul 17, 2022
@expipiplus1
Copy link
Copy Markdown
Contributor

Is this still on the table?

@stale
Copy link
Copy Markdown

stale bot commented Dec 3, 2022

Thank you for your contribution! I marked this pull request as stale due to inactivity. Please read the relevant sections below before commenting.

If you are the original author of the PR

  • GitHub sometimes doesn't notify people who commented / reviewed a PR previously when you (force) push commits. If you have addressed the reviews you can officially ask for a review from those who commented to you or anyone else.
  • If it is unfinished but you plan to finish it, please mark it as a draft.
  • If you don't expect to work on it any time soon, please consider closing it with a short comment encouraging someone else to pick up your work.
  • To get things rolling again, rebase the PR against the target branch and address valid comments.

If you are not the original author of the PR

  • If you want to pick up the work on this PR, please create a new PR and indicate that it supercedes and closes this PR.

@stale stale bot added the status: stale label Dec 3, 2022
@sumnerevans sumnerevans requested a review from berbiche December 8, 2022 16:00
@stale stale bot removed the status: stale label Dec 8, 2022
@stale
Copy link
Copy Markdown

stale bot commented Mar 8, 2023

Thank you for your contribution! I marked this pull request as stale due to inactivity. Please read the relevant sections below before commenting.

If you are the original author of the PR

  • GitHub sometimes doesn't notify people who commented / reviewed a PR previously when you (force) push commits. If you have addressed the reviews you can officially ask for a review from those who commented to you or anyone else.
  • If it is unfinished but you plan to finish it, please mark it as a draft.
  • If you don't expect to work on it any time soon, please consider closing it with a short comment encouraging someone else to pick up your work.
  • To get things rolling again, rebase the PR against the target branch and address valid comments.

If you are not the original author of the PR

  • If you want to pick up the work on this PR, please create a new PR and indicate that it supercedes and closes this PR.

@stale stale bot added the status: stale label Mar 8, 2023
@Dioprz
Copy link
Copy Markdown
Contributor

Dioprz commented Apr 30, 2023

I was bitten by this issue after release-21.11. I remap my leader from \ to Space, and since it seems programs.neovim.extraConfig is called later than the plugins my config stopped working (it used to work before).

For now I work around the same Issue by prepanding the first plugins configuration with my nvimrc. E.g.

programs.neovim = {
  ...
  plugins = [
    {
      plugin = pkgs.vimPlugins.gruvbox;
      config = builtins.readFile ./config/nvimrc + builtins.readFile ./config/gruvbox.vim;
    }
  ];
};

Thank you for sharing your answer, it saved me a lot of time.
In case it helps anyone else, if you have your entire configuration defined in an init.lua, you can separate it into 2 groups, the "core" configurations (that you need to run at the beginning) and the plugins config. That done, you will end up with something like:

programs.neovim = {
      ...
      extraConfig = ":luafile ~/.config/nvim/append_config.lua";
      plugins = with pkgs.vimPlugins; [
        { plugin = gruvbox-nvim;
          config = ("lua << EOF\n" + builtins.readFile ./nvim/preppend_config.lua + "EOF");
        }
       ...
       nvim-cmp
       etc
        ...
     ];
};

Where preppend_config will load the core, and append_config will take care of the plugins at the end.

However, this feels quite convoluted. would it be possible for you to please reconsider the initExtra option? I really consider it an option that carries enough value to create it!

Anyway, thank you and best regards!

@stale stale bot removed the status: stale label Apr 30, 2023
@expipiplus1
Copy link
Copy Markdown
Contributor

FWIW, I use this almost trivial patch. If someone wants to neaten it up (change the name to something more descriptive than initExtra, allow it to accept lua, add tests) then please feel free to take it.

diff --git a/modules/programs/neovim.nix b/modules/programs/neovim.nix
index df4b487d..28abf1e0 100644
--- a/modules/programs/neovim.nix
+++ b/modules/programs/neovim.nix
@@ -98,6 +98,14 @@ in {
     programs.neovim = {
       enable = mkEnableOption "Neovim";

+      initExtra = mkOption {
+        type = types.path;
+        default = null;
+        description = ''
+          Configuration to insert at the top of <filename>$XDG_CONFIG_HOME/nvim/init.vim</filename>
+        '';
+      };
+
       viAlias = mkOption {
         type = types.bool;
         default = false;
@@ -401,6 +409,8 @@ in {
         (map (x: x.runtime) pluginsNormalized) ++ [{
           "nvim/init.lua" = let
             luaRcContent =
+              lib.optionalString (cfg.initExtra != null)
+              "vim.cmd [[source ${cfg.initExtra}]]\n" +
               lib.optionalString (neovimConfig.neovimRcContent != "")
               "vim.cmd [[source ${
                 pkgs.writeText "nvim-init-home-manager.vim"

@ncfavier
Copy link
Copy Markdown
Member

ncfavier commented May 1, 2023

Why not extraConfig = mkBefore ...?

@stale
Copy link
Copy Markdown

stale bot commented Jul 30, 2023

Thank you for your contribution! I marked this pull request as stale due to inactivity. Please read the relevant sections below before commenting.

If you are the original author of the PR

  • GitHub sometimes doesn't notify people who commented / reviewed a PR previously when you (force) push commits. If you have addressed the reviews you can officially ask for a review from those who commented to you or anyone else.
  • If it is unfinished but you plan to finish it, please mark it as a draft.
  • If you don't expect to work on it any time soon, please consider closing it with a short comment encouraging someone else to pick up your work.
  • To get things rolling again, rebase the PR against the target branch and address valid comments.

If you are not the original author of the PR

  • If you want to pick up the work on this PR, please create a new PR and indicate that it supercedes and closes this PR.

@stale stale bot added the status: stale label Jul 30, 2023
@expipiplus1
Copy link
Copy Markdown
Contributor

@ncfavier because extraConfig itself is always placed after the plugin configs, so even if something is at the top of extraConfig it's too late.

@stale stale bot removed the status: stale label Aug 19, 2023
@@ -1,3 +1,7 @@
" generated by Home Manager
set nocompatible
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

neovim sets nocompatible by default so it shoulkd not appear

@teto
Copy link
Copy Markdown
Collaborator

teto commented Aug 19, 2023

ideally nixpkgs' code should be rewritten with the nixos module so that we dont have to create extra options and can just do mkBefore/mkAfter.

I am counting on NixOS/nixpkgs#237425 to make overrides easier too.

@stale
Copy link
Copy Markdown

stale bot commented Nov 18, 2023

Thank you for your contribution! I marked this pull request as stale due to inactivity. Please read the relevant sections below before commenting.

If you are the original author of the PR

  • GitHub sometimes doesn't notify people who commented / reviewed a PR previously when you (force) push commits. If you have addressed the reviews you can officially ask for a review from those who commented to you or anyone else.
  • If it is unfinished but you plan to finish it, please mark it as a draft.
  • If you don't expect to work on it any time soon, please consider closing it with a short comment encouraging someone else to pick up your work.
  • To get things rolling again, rebase the PR against the target branch and address valid comments.

If you are not the original author of the PR

  • If you want to pick up the work on this PR, please create a new PR and indicate that it supercedes and closes this PR.

@stale stale bot added the status: stale label Nov 18, 2023
@teto
Copy link
Copy Markdown
Collaborator

teto commented Nov 21, 2023

dont update this PR yet, there is another approach I would like to take now that most of it is merged in nixpkgs.

@stale stale bot removed the status: stale label Nov 21, 2023
@stale
Copy link
Copy Markdown

stale bot commented Feb 20, 2024

Thank you for your contribution! I marked this pull request as stale due to inactivity. Please read the relevant sections below before commenting.

If you are the original author of the PR

  • GitHub sometimes doesn't notify people who commented / reviewed a PR previously when you (force) push commits. If you have addressed the reviews you can officially ask for a review from those who commented to you or anyone else.
  • If it is unfinished but you plan to finish it, please mark it as a draft.
  • If you don't expect to work on it any time soon, please consider closing it with a short comment encouraging someone else to pick up your work.
  • To get things rolling again, rebase the PR against the target branch and address valid comments.

If you are not the original author of the PR

  • If you want to pick up the work on this PR, please create a new PR and indicate that it supercedes and closes this PR.

@stale stale bot added the status: stale label Feb 20, 2024
obreitwi added a commit to obreitwi/dot_nixos that referenced this pull request Jun 10, 2024
* hard-code mapleader for now until
  nix-community/home-manager#2391 is resolved
@thiagokokada
Copy link
Copy Markdown
Contributor

@teto Any updates here?

@stale stale bot removed the status: stale label Aug 5, 2024
@stale
Copy link
Copy Markdown

stale bot commented Nov 14, 2024

Thank you for your contribution! I marked this pull request as stale due to inactivity. Please read the relevant sections below before commenting.

If you are the original author of the PR

  • GitHub sometimes doesn't notify people who commented / reviewed a PR previously when you (force) push commits. If you have addressed the reviews you can officially ask for a review from those who commented to you or anyone else.
  • If it is unfinished but you plan to finish it, please mark it as a draft.
  • If you don't expect to work on it any time soon, please consider closing it with a short comment encouraging someone else to pick up your work.
  • To get things rolling again, rebase the PR against the target branch and address valid comments.

If you are not the original author of the PR

  • If you want to pick up the work on this PR, please create a new PR and indicate that it supercedes and closes this PR.

@teto
Copy link
Copy Markdown
Collaborator

teto commented Nov 14, 2024

this will become obsolete once #5964 gets merged (after the 24.11 release)

@teto teto closed this Nov 14, 2024
obreitwi added a commit to obreitwi/dot_nixos that referenced this pull request Mar 6, 2026
* hard-code mapleader for now until
  nix-community/home-manager#2391 is resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.