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

CSpell Multi Config Support #62

Merged
merged 1 commit into from
Aug 14, 2024
Merged

Conversation

JateNensvold
Copy link
Contributor

Adds functionality discussed in #61

@davidmh
Copy link
Owner

davidmh commented Jul 17, 2024

Hi @JateNensvold the idea makes sense, before I take a deeper look, could you address the failing tests?

@JateNensvold
Copy link
Contributor Author

Yeah! The cspell.nvim tests were actually passing before my last commit, I must have forgotten to recheck after my last change. Ill take a look as soon as I have some free time this weekend and ensure they are passing in the next commit as well as address the commit message/lua style failures.

@JateNensvold
Copy link
Contributor Author

Sorry for the delay. I addressed the failures that I saw in the approval workflows and they should be passing based on what I see locally.

I expect there might be some more changes needed based on your thoughts on some of the rewrites I did to some of the core helper functionality to add support for multi config and configs outside the project directory.

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.

There are a couple for formatting issues I noticed. It also looks like the tests are not passing.

I pulled these two helpers from my personal config, I think they might help with the titles for the code actions:

--- @param path string
--- @return string
M.shorten_path = function(path)
    return Path:new(path):expand():gsub(Path:new("."):expand(), "."):gsub(vim.env.HOME, "~")
end

--- Formats a string using a table of substitutions.
--- E.g. `M.format("Hello ${subject}", { subject = "world" })` returns `Hello world`
---
--- @param str string The string to format
--- @param tbl table k-v pairs of string substitutions
--- @return string, number
M.format = function(str, tbl)
    ---@param param string
    return str:gsub("$%b{}", function(param)
        return (tbl[string.sub(param, 3, -2)] or param)
    end)
end

tests/spec/diagnostics_spec.lua Outdated Show resolved Hide resolved
lua/cspell/code_actions/make_add_to_dictionary_action.lua Outdated Show resolved Hide resolved
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 also noticed it this message every time I start a neovim session:

Created a new cspell.json file at $HOME/.cache/nvim/cspell.nvim/cspell.json

Looks like the file is created unconditionally every time. That doesn't sound right.

@JateNensvold
Copy link
Contributor Author

I also noticed it this message every time I start a neovim session:

Created a new cspell.json file at $HOME/.cache/nvim/cspell.nvim/cspell.json

Looks like the file is created unconditionally every time. That doesn't sound right.

Looks like this was an unintended side effect of creating a merged cspell config that imports the other configs. I added some additional checks so the config gets/cached on disk, and is only recreated when additional config(s) have been added to the nvim setup.

@davidmh
Copy link
Owner

davidmh commented Aug 5, 2024

I'll give it another look later this week.

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.

It's looking great, I commented on a couple of things we should address before merging.

lua/cspell/code_actions/make_add_to_json.lua Outdated Show resolved Hide resolved
lua/cspell/helpers.lua Outdated Show resolved Hide resolved
lua/cspell/helpers.lua Outdated Show resolved Hide resolved
@JateNensvold JateNensvold force-pushed the multi-config branch 2 times, most recently from 93a54b0 to 9127d66 Compare August 13, 2024 07:47
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.

Works great, thanks!

@davidmh davidmh merged commit c423159 into davidmh:main Aug 14, 2024
4 checks passed
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