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

fix: some fixes for use with Neovim #104

Closed
wants to merge 1 commit into from

Conversation

mrcjkb
Copy link

@mrcjkb mrcjkb commented Aug 30, 2024

This PR adds the following fixes for use with Neovim:

  • Make sure the plugin script can only be sourced once (this is a best practise for plugins).
  • Move the registration of the nu filetype to an ftdetect/nu.lua script, as is common practise for Neovim plugins.
  • Move setting the commentstring for nu files to ftplugin/nu.lua (so there's no need to create an autocommand).
  • Make sure the plugin doesn't error if nvim-treesitter isn't installed. This is in preparation for packaging this parser for luarocks.org, so that rocks.nvim/luarocks users can use it without having to install nvim-treesitter.
    See also: Publish SemVer tags to luarocks.org? #103

@fdncred
Copy link
Collaborator

fdncred commented Aug 30, 2024

Thanks for trying to make this better. However, I'm not sure I'm a fan of this PR since it makes it more complicated for people to understand what to do. I use nvim and don't have ftdetect or ftplugin at all so it makes me think that this is for some specific configuration that the average nvim user may not have. We're trying to keep it real general for the common user.

Maybe there's a way to add this stuff separately "for the x type of install" and still keep the original one? 🤷🏻‍♂️

I'm fine with fixes though if there are problems with the script. Let's see what others say.

@mrcjkb
Copy link
Author

mrcjkb commented Aug 30, 2024

:h ftdetect and :h ftplugin are built-in Neovim features that are enabled by default.

There's nothing users need to do for this to work. Neovim sources the scripts automatically at the appropriate times (as opposed to sourcing everything at startup, regardless of whether a nushell file has been opened or not, which is what the current implementation does).

@fdncred
Copy link
Collaborator

fdncred commented Aug 30, 2024

I'm also wondering if this stuff needs to go in our integrations repo vs here with a readme linking to the integrations repo.

@mrcjkb
Copy link
Author

mrcjkb commented Aug 30, 2024

That sounds reasonable to me.

@fdncred
Copy link
Collaborator

fdncred commented Aug 30, 2024

I'd accept it in integrations if

  1. you make a couple folders to leave the original one file implementation
  2. you make a new folder for these changes with a readme that explains why it exists

Then you could come back here and add a readme.md to the plugin folder that points there.

should we move this one file implementation over there in yet another folder?

@mrcjkb
Copy link
Author

mrcjkb commented Aug 30, 2024

I don't understand your suggestion. I don't use this parser myself, I'm just responding to a request to get it packaged.

Come to think of it, I believe the proper way to go for filetype detection is to upstream the .nu filetype to vim (which would result in it being backported to Neovim eventually).

After that, the ftdetect and ftplugin scripts would be redundant.
Registering with nvim-treesitter isn't needed for rocks.nvim users, hence the pcall as they likely won't have nvim-treesitter installed. But I can just exclude the plugin script from the luarocks package.

@mrcjkb
Copy link
Author

mrcjkb commented Aug 31, 2024

Turns out vim (and by extension neovim) has built-in filetype detection for nu.
It just doesn't set the commentstring, which I've opened a PR for: vim/vim#15601

@mrcjkb
Copy link
Author

mrcjkb commented Sep 1, 2024

the commentstring patch has been backported to Neovim.

Closing, as I can just exclude the plugin directory in the luarocks package.

My recommendation would be for you to remove the redundant filetype registration, to only set the commentstring if vim.fn.has('nvim-0.11') ~= 1 and to wrap the nvim-treesitter usage in a pcall.

@mrcjkb mrcjkb closed this Sep 1, 2024
@mrcjkb mrcjkb deleted the push-vtnrmusrowlq branch September 1, 2024 11:03
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.

2 participants