lewis6991 / spellsitter.nvim

Treesitter powered spellchecker
MIT License
417 stars 21 forks source link

Get parser correctly and depend on nvim-treesitter #69

Closed seblj closed 2 years ago

seblj commented 2 years ago

This is a proposal on how to fix #68.

Let me know what you think

seblj commented 2 years ago

noticed that ci broke because of treesitter dependency. will try to fix this

lewis6991 commented 2 years ago

I think I'd prefer to keep nvim-treesitter as a soft dependency for now. So use its modules when they are available, otherwise use our vendored versions.

seblj commented 2 years ago

Okay, do you think we need to implement our own ft_to_lang, or just say that if someone else runs into this problem, then they have to install nvim-treesitter to fix this?

seblj commented 2 years ago

Probably the easiest solution would be to just create a table to map the filetype to the parser name for those languages that spellsitter supports, and where the parser name is not equal to the filename

lewis6991 commented 2 years ago

Either, I don't mind.

99.9% of the time, nvim-treesitter is going to be available.

Making it a soft dependency just makes testing a bit easier.

seblj commented 2 years ago

Agreed. I think just having a table with the languages spellsitter supports would be easier if nvim-treesitter is not available. Because it looks like it would be a lot of code to replicate the function.

lewis6991 commented 2 years ago

Sure, that or just don't provide one and pass nil to get the old behaviour when nvim-treesitter isn't available. Your call.

seblj commented 2 years ago

I just pass in the filetype if nvim-treesitter isn't installed since that is the default for get_parser

lewis6991 commented 2 years ago

LGTM

Will merge later when not on mobile

clason commented 2 years ago

Nvim-treesitter maintainer here: I think not depending on nvim-treesitter if you don't have to is the right choice that will save you heartbreak in the long run; a significant proportion of the utility methods from nvim-treesitter are also in the process of being upstreamed to Neovim, at which point they'll be fine to depend on. (This may include a "filetype-to-parser mapping, although I think this is a bit too closely tied to parser management which is and will remain nvim-treesitter's business.)

In particular, since spellsitter provides its own queries (so won't work without explicit support for a language?), just maintaining your own table for those few languages that need them sounds like the easiest and best option.

seblj commented 2 years ago

In particular, since spellsitter provides its own queries (so won't work without explicit support for a language?), just maintaining your own table for those few languages that need them sounds like the easiest and best option.

I understand. I can create a table that languages that we can add to if queries for more languages is added in the future.

lewis6991 commented 2 years ago

In particular, since spellsitter provides its own queries (so won't work without explicit support for a language?), just maintaining your own table for those few languages that need them sounds like the easiest and best option.

It does create its own queries, but also defaults to queries in nvim-treesitter (via comment captures) if we don't provide any. Basically means spellsitter will work well enough for 95% of languages.

clason commented 2 years ago

It does create its own queries, but also defaults to queries in nvim-treesitter (via comment captures) if we don't provide any. Basically means spellsitter will work well enough for 95% of languages.

I see. Then you do want to use that map if it's available (and possibly fall back to your own that only covers the languages you have queries for).

lewis6991 commented 2 years ago

For reference https://github.com/lewis6991/spellsitter.nvim/blob/master/lua/spellsitter.lua#L159-L176

seblj commented 2 years ago

I added a table that could serve as a fallback for the few (if any) users that don't have nvim-treesitter. I could just see latex for the langues in queries/ that had a different parser name than filetype

seblj commented 2 years ago

I'm not sure why the CI failed though. It doesn't seem like it's related to what I did from what I can see

seblj commented 2 years ago

Do you have any ideas on why the tests are failing @lewis6991? It looks like there is something with the screen testing module from what I can see, but I have no idea how that works, and I can't seem to figure it out. I tried to run the tests locally both with and without the changes I made in this PR, but it still failed. It might be that I don't run the tests correctly locally, but I do get the same error there

lewis6991 commented 2 years ago

CI looks very stale and fails for me too. Not really sure what's happened since it uses a tag. I'll need to fix it first before we can merge this.

seblj commented 2 years ago

Totally understand👍