doom-neovim / doom-nvim

A Neovim configuration for the advanced martian hacker
GNU General Public License v2.0
1k stars 108 forks source link

Fixes for transitioning to lazy.nvim #421

Closed bavalpey closed 1 year ago

bavalpey commented 1 year ago

I looked through the various files and changed everywhere that was still using logic from packer to now use the equivalent from lazy.nvim.

In particular, this fixes the slew of warnings that are reported by Lazy: health, namely by changing all of the after = ... remnants from packer to use dependencies = { ... }. The README for lazy.nvim states that this isn't needed for most cases, but fails to explain why. I'm not 100% sure on the use cases for the after argument, so I left it in.

The above fixes also changed the key field in lua/doom/modules/features/telescope/init.lua to keys, and changed the reference to spec.lock in lua/doom/core/modules.lua to spec.pin. This removes all of the warning messages I had, although I have not enabled all of the different modules to test so perhaps this should be more closely examined.

Many things are still broken. Namely, the LspInstall command is not functional. I tried a couple of changes to lua/doom/modules/features/auto_install/init.lua which replaces the reference to packadd with the lazy equivalent. However, this did not end up working, as after the change, typing LspInstall [server] results in the message:E488: Trailing characters: [server]. If you do not pass in an argument, then the command seems to work in listing the available servers for the file type being edited. However, exiting out of that menu with q erroneously returns an error.

molleweide commented 1 year ago

Hmm, when I merged latest main into my daily driver branch, I don't think I experienced any of the issues you are listing above. The transition went perfectly.

connorgmeehan commented 1 year ago

This is amazing thanks @bavalpey. Everything looks good except for the after -> dependencies problem. You're welcome to fix the require("lazy").load(...)/fix null-ls not lazy loading/ fix LspInstall/LspUninstall issue but I'm happy to merge the PR and resolve them myself tomorrow :).

Edit: Actually upon re-reading your message maybe we're getting different errors for the LspInstall/LspUninstall. What device are you running on? I have mainly used macos in the past but I just got a linux tower that I need to test it on (still using the stable release of doom-nvim for work however). I'll try it out tomorrow.

bavalpey commented 1 year ago

This is amazing thanks @bavalpey. Everything looks good except for the after -> dependencies problem.

Do you mean switching to dependencies isn't needed? (I wasn't sure so I just made the change, although if I understood lazy.nvim's docs, having it in when not needed doesn't hurt, it is just useless). We need to remove the after keyword as it isn't supported by lazy.nvim.

You're welcome to fix the require("lazy").load(...)/fix null-ls not lazy loading/ fix LspInstall/LspUninstall issue but I'm happy to merge the PR and resolve them myself tomorrow :).

I don't know what you mean regarding null-ls not lazy loading. If you are referring to the change I made in lua/doom/modules/langs/utils.lua, that was not because of null-ls not lazy loading, but because this portion uses packadd which doesn't work.

I couldn't figure out the LspInstall/Uninstall issue. See below

Edit: Actually upon re-reading your message maybe we're getting different errors for the LspInstall/LspUninstall. What device are you running on? I have mainly used macos in the past but I just got a linux tower that I need to test it on (still using the stable release of doom-nvim for work however). I'll try it out tomorrow.

We probably aren't getting different errors. The errors I listed happened after I tried various fixes to doom's LspInstall command. The error as is in the current repo has to do with the packadd command.

Also, according to the README for mason.nvim, mason.nvim shouldn't be lazy loaded. We need to add a commit that adds lazy = false to wherever it's being loaded.

If there's anything else specific you need me to change for the PR to go through, let me know so I can do it.

edit: The device that I'm experiencing these issues is running linux 18.04 (although this will be upgraded to 20.04 in the future).

FWIW I have several different devices that I use on a semi-regular basis all of which I use doom-nvim on. I haven't updated them yet due to the issues, though.

bavalpey commented 1 year ago

Hmm, when I merged latest main into my daily driver branch, I don't think I experienced any of the issues you are listing above. The transition went perfectly.

Try on a fresh install (perhaps use docker)

bavalpey commented 1 year ago

Another thing to note is that mason.nvim adds several commands for installing and uninstalling packages it supports. E.g. MasonInstall. Should this be exposed to the user? I'm wondering if it will bypass the configuration that happens in doom's side (which is presumably why doom overrides the LspInstall command).

With whatever change, we should make sure these commands work on a fresh install. E.g. try using something like MasonInstall clang-format and ensure it works as expected.

molleweide commented 1 year ago

Hmm, when I merged latest main into my daily driver branch, I don't think I experienced any of the issues you are listing above. The transition went perfectly.

Try on a fresh install (perhaps use docker)

Yes, you might be right. Regardless, as Connor said above, thanks a lot for helping out with doom.

connorgmeehan commented 1 year ago

Do you mean switching to dependencies isn't needed? (I wasn't sure so I just made the change, although if I understood lazy.nvim's docs, having it in when not needed doesn't hurt, it is just useless). We need to remove the after keyword as it isn't supported by lazy.nvim.

You're 100% right, my bad! Keeping as is is good for now. :) I'll just merge current and tweak the lazy loading issues I referenced.

Also, according to the README for mason.nvim, mason.nvim shouldn't be lazy loaded. We need to add a commit that adds lazy = false to wherever it's being loaded.

No we don't lazy load mason.nvim but mason-lspconfig (thin wrapper that converts nvim-lsp-installer names to mason.nvim names) is safe to lazy load.

Also, looks like deps get lazy = false automatically if they meet certain criteria so no need to add to the package specs. (See here.

Another thing to note is that mason.nvim adds several commands for installing and uninstalling packages it supports. E.g. MasonInstall. Should this be exposed to the user?

Doom-nvim just uses mason.nvim to ensure that a dependency is installed. The user might want to downgrade / upgrade those dependencies themselves so I think it's worth still keeping these commands exposed.