Skip to content

Add initExtra option to neovim#2244

Closed
viniciusmuller wants to merge 1 commit intonix-community:masterfrom
viniciusmuller:master
Closed

Add initExtra option to neovim#2244
viniciusmuller wants to merge 1 commit intonix-community:masterfrom
viniciusmuller:master

Conversation

@viniciusmuller
Copy link
Copy Markdown

Description

This PR adds an initExtra option to neovim, allowing users to add vimscript configuration to the top of the generated init.vim,
without using workarounds such as #2213. The generated init.vim will have the following shape:

{initExtra}
{pluginsConfiguration}
{extraConfig}

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.

@viniciusmuller viniciusmuller requested a review from rycee as a code owner August 1, 2021 19:50
Comment thread modules/programs/neovim.nix
Comment thread modules/programs/neovim.nix Outdated
Comment thread modules/programs/neovim.nix Outdated
@teto
Copy link
Copy Markdown
Collaborator

teto commented Aug 4, 2021

I wouldn't consider the suggestions in the linked PR as "workarounds", don't they work for you. The goal should be to keep options to a minimum so I don't think this MR is needed ?

@viniciusmuller
Copy link
Copy Markdown
Author

I personally don't think that makes sense an user have to use xdg.configFile in order to set options on the top of their vimrc. The module should provide abstractions to do that. If you don't think that this is useful, that's ok. But the provided solution doesn't work for me and I would like to use upstream home-manager instead of my fork.

@berbiche
Copy link
Copy Markdown
Member

berbiche commented Aug 8, 2021

Hi @arcticlimer,

Can you set the manifestRc param of makeNeovimConfig to

''
" generated by Home Manager
set nocompatible
${cfg.initExtra}
''

and set back programs.neovim.generatedConfigViml = neovimConfig.neovimRcContent.

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.

@teto what do you think of the manifestRc solution?

Comment thread modules/programs/neovim.nix Outdated
Copy link
Copy Markdown
Contributor

@sumnerevans sumnerevans left a comment

Choose a reason for hiding this comment

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

Please format the commits according to the CONTRIBUTING guidelines

@viniciusmuller
Copy link
Copy Markdown
Author

Hey, @berbiche, are you sure that this manifestRc gets written to the top of the neovim config? I've updated my flake and gave it a try before formatting the commits but the strings inside initExtra doesn't seem to exist anywhere inside the generated init.vim after this change.

@berbiche
Copy link
Copy Markdown
Member

Hey, @berbiche, are you sure that this manifestRc gets written to the top of the neovim config? I've updated my flake and gave it a try before formatting the commits but the strings inside initExtra doesn't seem to exist anywhere inside the generated init.vim after this change.

I had a look at the neovim/vim code in nixpkgs again and I was mistaken.
I'm now 99% sure that changing manifestRc to configure.beforePlugins will achieve the correct result.

From neovim/utils.nix:

{
      configurePatched = configure // {
        customRC = pluginRc + customRC + (configure.customRC or "");
      };
      # <snip>
      manifestRc = vimUtils.vimrcContent (configurePatched // { customRC = ""; }) ;
}

From vim-plugins/vim-utils.nix:

{
  vimrcContent = {
    packages ? null,
    vam ? null,
    pathogen ? null,
    plug ? null,
    beforePlugins ? ''
      " configuration generated by NIX
      set nocompatible
    '',
    customRC ? null
  }:
  { };
}

@viniciusmuller
Copy link
Copy Markdown
Author

Could you elaborate a bit more? When trying to change manifestRc to configure.beforePlugins nix complains about confire.beforePlugins being redefined, because it was already defined at line 284. Even if I comment line 56, the error persists. Also, was there any specific problem with my previous solution?

@berbiche
Copy link
Copy Markdown
Member

Also, was there any specific problem with my previous solution?

The manifestRc solution does not work because its value never gets used.

Could you elaborate a bit more? When trying to change manifestRc to configure.beforePlugins nix complains about confire.beforePlugins being redefined, because it was already defined at line 284.

In that case, you can set beforePlugins on line 56 to the value of initExtra.

@berbiche
Copy link
Copy Markdown
Member

I cannot force push the patch to your branch @arcticlimer,

Here's the content:

commit 77d0a500cfead7e1a61d8c97de0cfe29c2dfb5f6
Author: Nicolas Berbiche <nicolas@normie.dev>
Date:   Sat Aug 21 23:00:20 2021 -0400

    neovim: move initExtra to beforePlugins block

diff --git a/modules/programs/neovim.nix b/modules/programs/neovim.nix
index 66bcb44..22ddc9a 100644
--- a/modules/programs/neovim.nix
+++ b/modules/programs/neovim.nix
@@ -53,7 +53,11 @@ let
         (map (x: if x ? plugin && x.optional == true then x.plugin else null)
           cfg.plugins);
     };
-    beforePlugins = "";
+    beforePlugins = ''
+      " generated by Home Manager
+      set nocompatible
+      ${cfg.initExtra}
+    '';
   };
 
   extraMakeWrapperArgs = lib.optionalString (cfg.extraPackages != [ ])
@@ -285,11 +289,6 @@ in {
       plugins = cfg.plugins
         ++ optionals cfg.coc.enable [ pkgs.vimPlugins.coc-nvim ];
       customRC = cfg.extraConfig;
-      manifestRc = ''
-        " generated by Home Manager
-        set nocompatible
-        ${cfg.initExtra}
-      '';
     };
 
   in mkIf cfg.enable {

I verified this PR worked on my local configuration.

@viniciusmuller
Copy link
Copy Markdown
Author

Is this right @berbiche? Also I really regret that I forgot to branch out from master when I created this PR.

Comment thread modules/programs/neovim.nix Outdated
@berbiche
Copy link
Copy Markdown
Member

@arcticlimer can you update the failing test? neovim-plugin-config

@viniciusmuller
Copy link
Copy Markdown
Author

I think this might work? Could not test perfectly on my machine, since it was failing due to being generated with other home-manger warnings, but the error about the "generated by home-manager" thing was not here.

@berbiche
Copy link
Copy Markdown
Member

berbiche commented Sep 7, 2021

The neovim-plugin-config test is still failing due to a missing blank line.
You can run this test individually with nix-shell --pure tests -A run.neovim-plugin-config (from the root of HM).

@viniciusmuller
Copy link
Copy Markdown
Author

I'm getting this locally, still, I can't see the missing newline error, and setting the mentioned option in my config does nothing. If you could point this change to be committed here, I would be glad :)

❯ nix-shell --pure tests -A run.neovim-plugin-config
neovim-plugin-config: FAILED
Expected home-files/asserts/warnings.actual to be same as /nix/store/36arwncfmf610vjfi8qk3rvvhag9s5gw-warnings.expected but were different:
--- actual
+++ expected
@@ -1,14 +0,0 @@
-You are using
-
-  Home Manager version 21.11 and
-  Nixpkgs version 21.05.
-
-Using mismatched versions is likely to cause errors and unexpected
-behavior. It is therefore highly recommended to use a release of Home
-Manager than corresponds with your chosen release of Nixpkgs.
-
-If you insist then you can disable this warning by adding
-
-  home.enableNixpkgsReleaseCheck = false;
-
-to your configuration.
For further reference please introspect /nix/store/b4q7h0p5ami8fbjrcsn46bbcw79wg9s1-nmt-report-neovim-plugin-config

@berbiche
Copy link
Copy Markdown
Member

berbiche commented Sep 7, 2021

This should work: nix-shell -I nixpkgs=channel:nixos-unstable --pure tests -A run.neovim-plugin-config

The documentation should be updated to reflect the need to run the tests against the unstable branch of nixpkgs for HM master.

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.

Could you add a news entry to the news.nix file? The format to follow is specified in the file and in docs/contributing.adoc.

beforePlugins = "";
beforePlugins = ''
" 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.

we want home-manager not to generate a file if nothing is configured, this goes against that. Also neovim has set nocompatible by default so no need to pass that.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Could you so please provide a better and working solution? This is what I and berbiche were able to make. All the other provided solutions until now lead to nix errors or nvim config duplication, and I do not see why I should write my config trying to work around this, since it's a home-manager bug that should be fixed.

Copy link
Copy Markdown
Member

@berbiche berbiche Sep 9, 2021

Choose a reason for hiding this comment

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

Something like this:

Suggested change
set nocompatible
} // optionalAttrs (cfg.initExtra != "") {
beforePlugins = ''
" Generated by Home Manager
${cfg.initExtra}
''
};

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ping @arcticlimer

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Hello, I've forgot about that PR. This PR is currently a mess (it has conflicts, I made it from the wrong branch...), do you mind if I close this and apply these changes in a new PR?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Go ahead!

'';
};

generatedConfigViml = mkOption {
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.

why move this ?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

This got accidentally moved when it got readded after deletion.

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.

4 participants