neovim-require-check-hook: auto discover modules to require#352277
Merged
khaneliman merged 3 commits intoNixOS:stagingfrom Dec 3, 2024
Merged
neovim-require-check-hook: auto discover modules to require#352277khaneliman merged 3 commits intoNixOS:stagingfrom
khaneliman merged 3 commits intoNixOS:stagingfrom
Conversation
github-actions
bot
added
6.topic: python
khaneliman
force-pushed
the
neovimRequireCheck
branch
2 times, most recently
from
af9fb5b to
185f781
Compare
github-actions
bot
removed
the
6.topic: python
khaneliman
force-pushed
the
neovimRequireCheck
branch
from
185f781 to
181859d
Compare
pkgs/applications/editors/vim/plugins/neovim-require-check-hook.sh
Outdated
Show resolved
Hide resolved
Contributor
GaetanLepage
left a comment
GaetanLepage
left a comment
There was a problem hiding this comment.
This is a very good idea. The diff looks clear and correct to me on the algorithmic aspect.
I think that the pros outweigh the cons. In my opinion this hook will really help strengthening the package set.
ofborg
bot
added
10.rebuild-darwin: 501+
khaneliman
added
the
2.status: wait for branch‐off
khaneliman
force-pushed
the
neovimRequireCheck
branch
2 times, most recently
from
4b003bc to
c054027
Compare
Member
|
haven't looked at it in details but before you spend more time on this, I just wanted to say I am skeptical about this (it pains me to be negative about someone motivated to contribute with the plugin infra resilience, which I am really excited about !). Loading the module doesn't mean the plugin works, especially if it's lazy loaded. sqlite.nvim passes the nvimRequireCheck yet it fails miserably to load sqlite.so. I think the effort would be best spent differently. |
Contributor
Author
I guess my hope wasn't for a bulletproof test, just a smoke test to see if anything obvious is wrong. |
khaneliman
removed
the
2.status: wait for branch‐off
Member
|
PerchunPak
suggested changes
Nov 9, 2024
Member
There was a problem hiding this comment.
Looked into some failures, so here are edge-cases to handle I found:
Noinit.lua, let's try to require all files that are inlua/withoutstop_on_first_success(seeadwaita-nvim)File isplugin/*.luainstead oflua/*.lua(seeai-vim)- (will continue)
I looked over some of the failures and added missing dependencies, see my progress here.
pkgs/applications/editors/vim/plugins/neovim-require-check-hook.sh
Outdated
Show resolved
Hide resolved
Contributor
Author
Thanks for looking into it more. I haven't looked at this more since I've been in ZHF mode so it's very appreciated. I'll try making those tweaks to see how it improves the situation. |
Member
|
I think we should strive to remove nvimRequireCheckHook rather than improve it. There are several plugins that have busted tests which we dont run (rest.nvim/neorg etc) . With #352738 and #352742 we will have envrionments where we can run the tests instead of the |
Contributor
I am not sure to follow you here. I agree that |
Member
Plugins, that have tests, are exceptions. Most of the vim plugins are just a few lua files and smoke test is fine for them. Also, writing our own tests for a plugin, is a lot of work. If we could implement a better solution for specific situations (e.g. automatic testing for colorschemes), we should prefer it instead of
I wouldn't want to force contributors to write tests for every single plugin, smoke test is fine for most of them. |
Contributor
|
Yes, I agree with @PerchunPak. Of course |
khaneliman
force-pushed
the
neovimRequireCheck
branch
2 times, most recently
from
1c88a11 to
9f3ec12
Compare
PerchunPak
suggested changes
Nov 12, 2024
pkgs/applications/editors/vim/plugins/neovim-require-check-hook.sh
Outdated
Show resolved
Hide resolved
13 tasks
GaetanLepage
approved these changes
Dec 1, 2024
Contributor
GaetanLepage
left a comment
GaetanLepage
left a comment
There was a problem hiding this comment.
I too am in favor of merging this PR.
I think that its benefits largely overpass its drawbacks/limitations.
13 tasks
wegank
added
12.approved-by: package-maintainer
khaneliman
force-pushed
the
neovimRequireCheck
branch
from
dd97075 to
0365ac4
Compare
github-actions
bot
removed
10.rebuild-darwin: 501+
Member
|
Run build for all vimPlugins again and nothing fails. Only |
13 tasks
khaneliman
force-pushed
the
neovimRequireCheck
branch
from
0365ac4 to
84b5bd5
Compare
Contributor
Author
|
Just waiting for the master -> staging merge before doing this. Looks like staging merge had conflicts so waiting on #360831 |
wegank
removed
12.approvals: 3+
13 tasks
ofborg
bot
added
10.rebuild-darwin: 501+
Member
I think the opposite ^^ it's a lot of code > 1k including the My philosophy is that people doing the work should have the final word and through nixvim you folks maintain the (neo)vim plugin set so if you are excited about it, go for it. |
GaetanLepage
approved these changes
Dec 3, 2024
Contributor
|
Good job @khaneliman for this great piece of work. It is an ambitious approach, I look forward to seeing how it behaves. Thank you to all the reviewers for providing their feedback and advice. |
13 tasks
teto
reviewed
Dec 16, 2024
teto
reviewed
Dec 16, 2024
| This can be manually added through plugin definition overrides in the [overrides.nix](https://github.com/NixOS/nixpkgs/blob/master/pkgs/applications/editors/vim/plugins/overrides.nix). | ||
| It accepts a single string for a module, or a list of module strings to test. | ||
| - `nvimRequireCheck = MODULE;` | ||
| - `nvimRequireCheck = [ MODULE1 MODULE2 ];` |
Member
There was a problem hiding this comment.
My personal preference would have been to accept juste one format to avoid surprising the user "it's a list here but not there ?", plus it makes implementation easier. Like always accept a list and mark it plural:
nvimRequireChecks = []
13 tasks
13 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR introduces automatic Lua module discovery and enhances the existing Lua module checking mechanism by enabling support for multi-module checks within Vim plugins. These changes improve the flexibility and usability of the module validation process for plugin maintainers, helping catch errors and confirm module integrity more effectively.
TODO:
- [ ] Cleanup redundant setting in overrides.nixKey Changes
Automatic Module Discovery:
If no specific modules are defined in nvimRequireCheck, the script now automatically searches the plugin source for conventional Lua module paths (e.g., lua/name/init.lua or standalone name.lua files).This ensures that module checking can occur even if a module list isn’t pre-populated, reducing the need for manual configuration.
Multi-Module Checking with Configurable Success Logic:
When nvimRequireCheck is pre-populated, the script now runs checks on all listed modules, providing comprehensive feedback on each module's status.In the discovery mode (when nvimRequireCheck starts empty), the script will stop after the first successful check, streamlining the process by stopping early when at least one valid module is confirmed.
Improved Output Formatting:
Summaries now use structured, indented bullet points, making the output more organized and easy to interpret.Drawbacks
Potential for Slower Execution:
If nvimRequireCheck is populated with many modules or if the automatic discovery mode finds a large number of potential modules, this can increase script execution time. The additional checks may slow down validation slightly, especially if running in a CI environment with many modules.
Overhead in Failure Cases:
If a plugin has many invalid or incomplete module paths, the script will attempt to check each one, potentially resulting in a lengthy list of failures before finding a valid module. This might require further optimization or fallback handling in future updates.
Edge Cases with Non-Standard File Structures:
The automatic discovery relies on conventional file structures (lua/name/init.lua or name.lua). Plugins that don’t follow these conventions may not have all relevant modules discovered, and maintainers might need to still specify nvimRequireCheck manually for non-standard cases.
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.