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: respect language settings of imported files #75

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

spacian
Copy link

@spacian spacian commented Jan 17, 2025

cspell only respects the language of the main config, thus making it impossible to add new languages exclusively by importing them. This pull request appends all the languages found in imported files to the main config automatically.

Fixes #74

cspell only respects the language of the main config, thus making it
impossible to add new languages exclusively by importing them. This
commit appends all the languages found in imported files to the main
config automatically.
@spacian
Copy link
Author

spacian commented Jan 17, 2025

One thing I noticed is that cspell normally allows trailing commas in json, this implementation does not. Maybe there is an easy way to fix that.

@spacian
Copy link
Author

spacian commented Jan 17, 2025

Another thing I noticed is that now the config needs a way to be reset. I only deleted the files manually, but that does not sound like a great way to do things, as it needs to be done every time a language is added.

I think it would be good to expose a function that resets / deletes the internal cspell.json for the current working directory, but I am kind of new to this and unsure on how to do that. Then every user could invoke the call as they please. E.g. I would probably just clear the file on VimEnter or SessionEnter.

Copy link
Owner

@davidmh davidmh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll do a more thorough review in the next few days; I don't have much bandwidth at the moment.

In the meantime, there are some minor linting issues, and I left a comment regarding the language property.

Exposing a cleanup function is going to be tricky, as we depend on the configuration params to build the combined config path. Maybe a new config option to build the combined file unconditionally?

and type(cspell_config.language) == "string"
and cspell_config.language ~= ''
then
cspell_json.language = cspell_json.language .. "," .. cspell_config.language
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Appending languages unconditionally would result in strings like en,en which is not necessarily a problem, but we should look into consolidating unique languages.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used your set_create to make the languages unique now.

@spacian
Copy link
Author

spacian commented Jan 18, 2025

I'm very thankful you're taking your time to look into this!

I see your point and I guess I'm mixing in a different issue... I think once the plugin is loaded, it just uses the first cspell file it found. However, I'm working a lot with sessions and when I switch my session, the plugin keeps using the old cspell.json. I feel like both of these things are connected and could be resolved simultaneously, but I could be wrong.

Just for my understanding, can params.cwd be different from vim.loop.cwd() or vim.fn.getcwd()? My naive mind thinks "the cwd is the cwd, no matter where I get it from."

An option to just force the rebuild every time (if that is what you mean) would probably be the easiest to implement. And at least on my machine, cspell takes like 1s to run anyway, another few milliseconds to rewrite a rather small file would hardly be noticeable.

Other than that I'll let my mind wander a little, maybe some other idea comes to mind. Have a nice weekend!

also moves from params.cwd to vim.fn.getcwd(-1,-1) (the global cwd)
because params.cwd is only set once to the first working directory
encountered and is never updated afterwards, no matter what happens to
the global cwd
@spacian
Copy link
Author

spacian commented Jan 19, 2025

I thought a little bit and also had to debug some stuff and I found out that the params.cwd never changes after it is set for the first time. This caused all the issues with the internal cspell.json not being updated on cwd changes, even when there was a new local cspell.json.

It also allowed me to introduce an option which forces a rewrite of the cspell.json when the global cwd changes. It is implemented by caching the last known working directory and forces a rewrite when the last know directory differs from the current working directory.

Last but not least I noticed occasionally that deep folder structures were generated for the internal cspell.json. I added another gsub to also replace \ with -. Now all the files are directly in the [cache_path]/cspell folder on windows with no further folders inside it, which looks correct to me, but maybe that was not your intention.

@davidmh
Copy link
Owner

davidmh commented Jan 19, 2025

Just for my understanding, can params.cwd be different from vim.loop.cwd() or vim.fn.getcwd()? My naive mind thinks "the cwd is the cwd, no matter where I get it from."

Not quite, params.cwd is set by the null-ls source, in case the directory
containing the CSpell config file is different from Neovim's current working
directory.

For example, working on a monorepo with the following structure:

.
├── cspell.json
├── service-a/
│   └── README.md
├── service-b/
│   └── README.md
└── service-c/
    └── README.md

You may want of focus on working on service-b, so that's where you open your
Neovim instance, but you still want to use the CSpell config at the root of the
repository.

For those cases, you can use the cwd property included in all null-ls
sources.
See: https://github.com/nvimtools/none-ls.nvim/blob/af318216b744f2fe2d5afa6febb01f455ae61d6c/doc/BUILTIN_CONFIG.md?plain=1#L293-L305

Using vim.loop.cwd or vim.fn.getcwd should be used as a fallback.

Using vim.fn.getcwd as the main cwd means that we would be ignoring that
setting, and it also causes the code actions to break if I'm running in a
directory different from the CSpell config file parent.

I can solve the issue by consuming cwd as:

local cwd = params.cwd or require('null-ls.utils').get_root()

I also noticed that changing the cwd keeps appending to the cached
configuration, which ends up mixing configurations across projectds.

For example, working on the following projects:

.
├── project-a/
│   └── cspell.json
├── project-b/
│   └── cspell.json
└── project-c/
    └── cspell.json

Let's say I start by editing project-c, then I switch to work on project-a,
by manually setting the cwd or using something like vim-rooter, now the code
actions include options for both projects. And if I then open project-b, I'll
get actions for all three projects.

That doesn't seem right.


Last but not least I noticed occasionally that deep folder structures were generated for the internal cspell.json. I added another gsub to also replace \ with -. Now all the files are directly in the [cache_path]/cspell folder on windows with no further folders inside it, which looks correct to me, but maybe that was not your intention.

I don't have a way to test against Windows, I would be comfortable with anything you decide on that, as long as it's covered by a test.

@spacian
Copy link
Author

spacian commented Jan 19, 2025

Thank you for your in-depth feedback!

It looks like I have a lot to learn about the plugin still. I'll look into it more, especially what the caches are doing and how that will interact with resets.

I feel like the usage of the params.cwd currently contradicts my personal usage of sessions because I just open nvim anywhere and then open my session, which also sets the cwd. But then params.cwd is never changed after being set once, because it always finds itself in subsequent calls even when I open a different project (i.e. a different session) in the same nvim instance. I'll probably have to check for session switches to reset params.cwd. That might also be a good point to reset the caches, however that is done properly. I'll figure it out, hopefully.

Thank you again for your time and effort!

also removes option to reset paths on cwd change as it caused unintended
behavior
@spacian
Copy link
Author

spacian commented Jan 25, 2025

My new approach is injecting reset_spell and cwd functions instead of exposing corresponding setters. This allows every user to be as flexible as they want with setting their cwd and resetting cspell. And while I was at it, I added the option also have default imports because I didn't want to always create a cspell.json for the German dictionary I'm using.

For example, I currently use this:

	local null_ls = require("null-ls")
	local cspell_last_session = ""
	null_ls.setup({
		fallback_severity = vim.diagnostic.severity.HINT,
		sources = {
			require("cspell").diagnostics.with({
				config = {
					cspell_import_files = function(_)
						return { vim.fn.expand("$APPDATA") .. "/npm/node_modules/@cspell/dict-de-de/cspell-ext.json" }
					end,
					cwd = function(_)
						return vim.fn.getcwd()
					end,
					reset_cspell = function(params)
						if params ~= nil and params.cwd ~= nil and vim.v.this_session ~= cspell_last_session then
							cspell_last_session = vim.v.this_session
							return true
						else
							return false
						end
					end,
				},
			}),
		},
	})

All the information in params is available because I pass params when calling cwd and reset_cspell.
Additionally, I'm clearing the caches now on reset. Whether that solves the issues you described I still have to check.

I also just now figured out how to escape all the right characters on windows to get the tests to run at all. Now I have to debug my runtime errors and also figure out what to test and how.

@spacian
Copy link
Author

spacian commented Feb 7, 2025

Unfortunately, I currently find very little time to work on this. I hope to in the future, but it's probably unrealistic at least in the next month or two. I'm not quite sure what to do with this merge request during that time. We could leave it as it is, I could open a new one when I think I made significant progress or convert it to a draft. Anything is fine by me, but I wanted to let you know.

@davidmh
Copy link
Owner

davidmh commented Feb 7, 2025

No worries, happy to review that new version whenever you're ready.

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.

cspell and cspell.nvim giving different results when importing cspell extensions
3 participants