Feature: restructure modules and config #277
Conversation
These changes have been discussed in doom-neovim#233. As a side note, I ran stylua and luacheck over the repo. * From everyone's perpective - The module structure has been flattened out, removing the category grouping. Beyond less iteration scopes, this may help with, in the future, allowing the user to write and enable custom modules in $DOOMDIR. * From the user's perpective - `doom_config.lua` works via overrides now, it can change defaults on the global `doom` before everything gets actually configured. - `doom_modules.lua` just returns the actual thing it's supposed to, without {return value}.source. More on that next. - Instead of each config file returning a source key with its file location, the handlers for each config file actively search for them. This is described in the respective files inside `lua/doom/core/config`, but it's basicaly a check for two special paths falling back to runtimepath. - `doom_userplugins.lua` is removed in favor of having its functionality in `doom_config.lua`. To add a standalone package, add its packer spec to `doom.packages` (this is a map-like table of package names to packer specs). - To override the spec of a module package, change the keys in `doom.packages[package_name_without_author]`. Special attrs: `config` is run after the built-in config. - To change settings and behavior, override keys in `doom[module]`, or just `doom.*` for some core features. - To add standalone bindings, add them to `doom.binds`. This is passed to nest along with the modules' binds. `doom.binds` overrides anything you don't like in modules, you can safely replace the bind. - To add standalone autocmds, add them to `doom.autocmds[augroup_name]`. The structure is as passed to `utils.create_augroups`, so: ```lua doom.autocmds[group_name] = { { event1, pattern1, cmd1 }, ... } ``` - You shouldn't override any tables, like `doom.autocmds` or `doom.packages`, only the leaves, else you remove all that is already there. We could use metatables to avoid this in the future. - The `config.nvim` table is no longer used, most of its functionality is spread elsewhere, like autocmds. For variables and options, just use vim.opt.*, vim.g.* etc. in `doom_config.lua` - Settings can also be appended to in `doom_config.lua`, because defaults are prepopulated. * From a maintainer's perpective - Most things are grouped under the `doom` global, which is populated and overriden early in `init.lua`. The exception is enabled modules, which you still need to require `doom.core.config.modules` to get. However, do so sparingly, and only if you truly mean to iterate over enabled modules disregarding user overrides (they may have removed a particular package or reset binds). - Modules are defined in the following folder structure inside `lua/doom/modules`: `<module>/{init.lua,packages.lua,binds.lua,autocmds.lua}`. init.lua and packages.lua are required, even if empty. - `init.lua` returns: defaults under a `defaults` key; config functions under `packer_config[package_name]`. It can access the doom global only inside the configs. - `packages.lua` returns: a map of package names to package specs, with no `config` or `disable` keys present. Most things should be lazy-loaded, preferably with the `cmd` and `module` keys. It cannot access the doom global. - `autocmds.lua` returns: a list of autocmds to be applied under the group `doom_{module}`. It can use the `doom` global to add things conditionally. - `binds.lua` returns: the keyconfig to be passed to nest. It can use the `doom` global to add things conditionally. What's left: - Implement lsp wrapping. - Document the individual modules. - Write a migration script.
This adds wrapping of lsp for one language: lua. It adds two modules, `auto_install` and `lua`. The former toggles auto installation, and is sort of a marker module. Beyond installing and configuring nvim-lsp-installer and DAPInstall.nvim, it doesn't really do anything. It's purpose is to be checked on the lua module which will use the installers if it is enabled. Otherwise just falls back to $PATH. Since I'm on NixOS, I can only test the non-auto installation, but it seems to be downloading properly for the auto version. The lua module also installs additional packages, namely lua-dev.nvim, which used to be part of the lsp module. It could add keybinds, but I couldn't think of any. It has a special autocommand that `dofile`s the config.lua, which actually sets up lsp (and possibly dap) lazily and only after the first lua file is opened. This commit is meant as a template/prototype to other language wrappings.
Also remove the built-in modules in favor of putting them inside utils (reloader and async) or implementing them later with the new module structure (others).
Now supports lua functions. The ideas come from @caligian and his repo, though modified to use a unique index instead of requiring a name.
LuigiPiucco
force-pushed
the
feature/module-architecture
branch
from
d738f3e
to
d1f7604
Compare
|
The linter and style checker should be pleased now. |
|
Hey sorry for the delay reviewing, getting around to it now. These are some smaller issues I found on a first read through. Before I get more into it I just want to confirm I understand how everything works and make sure I fully understand the lifecycle of a module. I'll write my understanding below, could you confirm if I'm right or correct me if I'm wrong? By the way, some really cool optimisations here, it's awesome to see. First
Then Then
|
That's correct, the only mistake in the explanation is a typo in the module name (it's
Yes and, in fact, it's done automatically if the user tries to override config in The exact method for running the two of them is simple, even though it took me a while to figure out. I first tried to create an anonymous function that just runs both in sequence, but them I ran into a packer issue that prohibits from having captures in config. I hit my head on the wall for a while, until I discovered that passing both functions in a table works, even though its undocumented. |
|
Ok, I noticed you implemented the lua language using a |
|
In I made an exception for the installation of treesitter parsers, they don't need the autoinstall module, because they aren't packaged in distros, at least that I know of. Additionally, the module breaks lsp and dap on NixOS, since the downloaded executables don't work on NixOS. An alternative solution would be to add three autoinstall modules, one for each of lsp, dap and treesitter. |
NTBBloodbath
added
priority: high
|
It looks like there are some issues with inter-module dependencies ( Firstly I think the enabled modules should be available before parsing the Secondly I think all overrides should be editable the same, logical way. I think autocommands, packer specs, keybinds, configure function should be under the same local lsp_module` = doom.modules.lsp
lsp_module.packages['lspconfig'].commit = '...'
lsp_module.keybinds = {...}
tbl.insert(lsp_module.autocmds, { ... })Lastly I think this This is just one possible solution and I'm interested in hearing alternatives to either of these issues. I think that this new architecture needs to be a bit clearer in how it functions and I think these changes would make a very clear mental modal of:
Edit: Again, great work! Sorry for the delay, I've had a very lazy new years and I'm now back at work but slowly chipping through reviewing this PR :). |
|
I've branched off your changes here and I've pushed some fixes/changes to some of the issues outlined here. If you're happy with my code, feel free to cherry pick from my branch. I'd like to get this PR to a point where I can use it for work and keep providing bugs and changes from more intense usage. I wouldn't cherry pick the new languages just yet, that can be done at the end. Also any commit with the |
This is true and, because I had already opened the PR when I noticed, I left it there to allow you to review without stuff changing under your feet. However, I think the solution is even simpler:
This is more of a stylistic choice, and while it does look a bit better, it incurs iteration cost and complexity. I can do it, but it would be a big force-push, since all the module looping would change. I will let us discuss some more before tackling this.
I don't think this is doable, not because of efficiency, but because we need
It is certainly valuable, but I'm assuming here you mean adding a print in
Yes, I've come to this conclusion as well, and I'm now thinking if I should add a documentation commit in this PR instead of saving it for later. I may push it along with the other stuff.
Most of these look great (I've not looked at the temp and lang ones yet), I can add them once I push with the other changes mentioned. |
|
I think it's best not to rebase from here on out just to maintain the commit messages and show the changes a little clearer. The only reason we'd want to rebase is to keep semantic commits but for such a large change I think it's better we just write the changelog ourselves.
Oh great! I had a go fixing some of these but ran into some errors. Might have another go soon.
Yep that sounds good
Ahhh that is a problem, the only conditional binds are in
I think that's ok because users will be modifying the same data that they're vim.inspecting. I think it will still be clear to the user because they're changing the data and they control the order of execution. print(vim.inspect(doom.modules.lsp.binds)) -- Initial config
doom.module.lsp.binds = { ... custom binds ... }
print(vim.inspect(doom.modules.lsp.binds)) -- Modified configAnother option could be a telescope view that allows users to search through current module configs and open the vim.inspected data in a new buffer. Maybe it could even prepend the data with the code required to set the data, like if you telescoped for -- This shows the current keybind settings for the telescope module.
-- Copy and paste this into your `config.lua` file to modify these.
doom.modules.telescope.keybinds = {
-- <default keybinds>
}
Yeah would love to see some documentation, might even help me with my understanding of how everything works. I think short clear documentation is indicative of a logical, good API so the documentation will probably also help us make API decisions. If you do a first pass I can come in and edit this documentation as well :). |
Previously ran on BufNewFile,BufRead.
Fixes issues with the kommentary module and provides new behaviour such as `gcA` to add comment to end of line.
This replaces broken `nvim-tree-docs`.
Previously, some modules would cause packer errors by depending on packages of disabled modules. This fixes that behavior.
It is more up to date and well maintained than my own, plus includes what I had implemented.
This is sometimes allowed, but here it would try and get things that weren't initialized.
|
Will this be merged? Looking forward to these changes |
|
Hey NextAlone, I have been expanding on these changes, adding language support, fixing bugs and writing documentation and I should be merging to The PR is here but it's a bit out of date (I just did some major refactoring to simplify the internal workings) #313 I will merge my changes, do some optimisations (addressing long packer sequencing time) make sure the documentation is good and then release it. Can I ask for some feedback on the documentation later on? |
Ok, you can, and it works fine |
For more info on this, see #233 and the commit messages, but essentially this separates concerns about the various functionalities: each module has its directory, toggling a module just entails listing it in
doom-nvim/modules.lua, everything else can be overridden and configured indoom-nvim/config.lua. Packages can be added per module (in apackages.luafile) or by the user in thedoom.packagestable, they can also override module packages however desired.The config files changed names as well, the
doom_prefix was removed, for simplicity, and we have the luxury of changing this because the old files wouldn't work anyway.These are big, breaking changes, which will be documented in later work. The next big thing to do here is add a README.md per module explaining all options, bindings and autocmds, plus whatever extra things they enable. Another extra piece of work is to create LSP wrapper for each language, with appropriate packages and config. Also, doom.norg should probably be rewritten, I imagine most of it is outdated now.
Also, I'd like to thank @connorgmeehan, @caligian and @NTBBloodbath for support and ideas.
Closes #233 #251
(and probably many others, but I'll list only direct fixes)