Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Registration failed for ruff and shfmt #5

Closed
trkwyk opened this issue May 30, 2024 · 6 comments
Closed

Registration failed for ruff and shfmt #5

trkwyk opened this issue May 30, 2024 · 6 comments

Comments

@trkwyk
Copy link

trkwyk commented May 30, 2024

Thank you for the fantastic plugin! I've always wanted to find some plugins that do auto registration after null-ls having been archived and finally I found this. I want to report some bug here. First is, some formatters are also linters like ruff so

local categories = utils_Set(spec.categories)
if categories['Formatter'] then
update_associations(associations, 'formatters', languages, spec.name)
elseif categories['Linter'] then
update_associations(associations, 'linters', languages, spec.name)
end
the else if logic is not necessarily true. Second is, I'm not sure using the language section in mason packages to register is the right way to go. For example, shfmt's language section shows Bash, Mksh and Shell whereas the file type for shell scripts (bash, sh, ash etc) is sh (zsh, csh are some special ones) so the formatters/linters don't register correctly. Sure for most of the languages this seems to be okay and I see you have a mapping module for exceptions but I don't really think that's the best practice.

@frostplexx
Copy link
Owner

frostplexx commented May 30, 2024

Thank you for your feedback.
As for your first point I fixed that issue and published a new release.
Regarding your second point I know that just guessing that the filetype and language isn't the most ideal approach but i couldn't think of a better one. mason-nvim-dap which I used as a reference while writing this plugin is also maintaining a mappings file:
https://github.com/jay-babu/mason-nvim-dap.nvim/blob/3614a39aae98ccd34124b072939d6283853b3dd2/lua/mason-nvim-dap/mappings/filetypes.lua#L4C1-L19C2
I'm also giving the users the ability to manually override/add definitions as mentioned in the readme
https://github.com/frostplexx/mason-bridge.nvim/blob/3fe9863bc99a9ddd8e99e8c5e217dd89ad062164/README.md?plain=1#L77C1-L87C3
If someone can think of a better approach for handling filetypes I'm happy to implement that.

@trkwyk
Copy link
Author

trkwyk commented May 30, 2024

Yes the filetype is a bit tricky. This just comes to my mind, maybe you could somehow utilise the vim.filetype.match api? https://github.com/neovim/neovim/blob/5c33815448e11b514678f39cecc74e68131d4628/runtime/doc/lua.txt#L2750
First you find some language to common extension mapping source, then map the mason languages to extensions, and then run vim.filetype.match({ filename="foo.extension" }) where foo is just some pseudo name (to fool around the filename and pattern check) and extension is the respective extension obtained last step. Well the api has mechanisms other than just extension to decide the file type (like inspecting the buffer content which we don't have but I'm not sure how it behaves if we supply the filename alone, it's certainly legit though) so ideally the extension obtained last step should be distinct enough or maybe you could tweak the table https://github.com/neovim/neovim/blob/5c33815448e11b514678f39cecc74e68131d4628/runtime/lua/vim/filetype.lua#L176 a bit and hard code that into the plugin to ensure unique results.
Anyway, this is just some of my ideas. Feel free to ignore them if that's too cumbersome it's certainly neither simple nor efficient. I imagine a more complete mapping list for the outliers is better if not always fail-safe. And even better let mason.nvim specify the filetype for every package just like lspconfig there would be no such hustle at all... The plugin as is is good enough! Feel free to close this issue. Thanks for the fix!

@frostplexx
Copy link
Owner

I don't think using vim.filetype.match is the way to go as we would have to still maintain a mapping but additionally have one extra translation step: Language -> Extension (manual mapping) -> Filetype. With the current approach the map simply directly translates Language -> Filetype, which is a bit simpler as it 1. Saves one computation step and 2. There can be many more extensions for one language than there are languages that are the same filetype at least in my experience.
Looking around a bit more what other plugins are doing they all maintain hardcoded mappings:
https://github.com/williamboman/mason-lspconfig.nvim/blob/a4caa0d083aab56f6cd5acf2d42331b74614a585/lua/mason-lspconfig/mappings/server.lua#L6C1-L216C2

I'm going to keep language handling as is for now and maybe switch it to a pure mappings based approach in the future if i discover that just using the language as the filetype doesn't work for too many languages.

@trkwyk
Copy link
Author

trkwyk commented May 31, 2024

Actually the mason-lspconfig mapping is not related to file type but rather as it said "Maps lspconfig server config name to its corresponding package name". However, because all the lsp servers ultimately have to be registered through lspconfig itself, and lspconfig has every server file type specified for example https://github.com/neovim/nvim-lspconfig/blob/b124ef3bd4435a6db7ff03ea2f5a23e1e0487552/lua/lspconfig/server_configurations/lua_ls.lua#L16 so ideally if mason.nvim could do this too all of mason-related repo maintainers could have an easier life.

@frostplexx
Copy link
Owner

Seems like i sent the wrong example. mason-lspconfig.nvim also has a map of filetype to language server https://github.com/williamboman/mason-lspconfig.nvim/blob/a4caa0d083aab56f6cd5acf2d42331b74614a585/lua/mason-lspconfig/mappings/filetype.lua#L3C1-L243C2
This is still not the same thing as I'm doing but its the same idea.
As to why mason.nvim doesnt do this i think its mostly because its outside of masons scope. They are a package manage which only downloads, updates and uninstalls its tools. The language parameter is not used anywhere except for search from what I've seen because nvim only works using filetypes. But I agree that it would be much more convenient for plugin devs if they included filetypes too.

@frostplexx
Copy link
Owner

In this comment williamboman/mason.nvim#593 (comment) the maintainer of mason.nvim mentions that language entries are only for presentational purposes.

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

No branches or pull requests

2 participants