WIP: feat: code action to add a misspelling to the config file #72

mikavilpas · 2024-06-12T19:22:04Z

Here's a draft on what could become the solution for #12

This adds a code action like this:

Selecting the code action will make the lsp server to edit the configuration file. Here's what the edits look like in this repository:

Requesting feedback

Some things are a bit open right now, and I'd like to discuss how they should be worked on:

Do you think this general idea is valid? Or should another idea be attempted?
The typos configuration can be overridden on each directory level, so I started a scheme that would allow choosing the configuration file to add the misspelling to. The starting directory (typically the project root, I guess?) is always suggested - the others should be suggested only if they exist. Do you think this is a good idea?
Tests obviously need to be added - I think this could be tested quite thoroughly as the existing tests looked really good.
Error handling is not as clean as I would like. I have worked in some OS rust projects that use anyhow, but I couldn't find a way to use it since the tower lsp crate requires a certain error type 😓 Maybe some trait implementations could be added that could automatically handle this?
Reloading the configuration has not been implemented in the project yet, as far as I can tell. I think this needs to be solved as currently the misspellings are ignored only after the LSP server has been restarted.

tekumara

This is great, thank you, and much how I'd approach it too! Some initial suggestions below:

tekumara · 2024-06-16T06:30:41Z

crates/typos-lsp/src/typos.rs

+ /// all config files are read by typos_cli, and the settings are applied in precedence order:
+ ///
+ /// <https://github.com/crate-ci/typos/blob/master/docs/reference.md>
+ pub fn config_files_in_project(


The way config resolution works is a bit gnarly.

typos cli will implicitly look for and use the first config file it finds in this order:

in the current working directory

in parents of the current working directory

If a config file is provided explicitly by --config then it will be used together with the implicit file found as per the above, and its settings will take precedence over the implicit file's.

typos lsp tries to mimic this behaviour as closely as possible. It accepts an explicit config file and needs to initialise an equivalent typos cli Instance and decide what it's path (ie: current working directory) should be. For path it uses the workspace folder provided when typos lsp is initialised (or the folders change). path is used to find the first implicit config file as the cli does above. We assume this best mimics how users run typos cli locally or in CI.

NB: it is possible, particularly with nvim, to initialise typos lsp without a workspace folder (see the conditions mentioned here). In these cases the current working directory is effectively set to the root of the filesystem. This means only a config file in the root of the filesystem will be used (together with an explicit config file).

Ok, so what?

Whether or not config resolution is currently optimal (and maybe its not for the case where we don't have a workspace folder) it probably makes sense to only offer config files that would actually be used by the Instance.

So I think that would mean offering all of:

the explicit config file, if it has been set.

a config file based in the current Instance's path, whether or not that config file exists.

(optional bonus) config files in ancestors of this path, but only if they already exist.

What do you think, does that makes sense?

I see - this is totally reasonable. I think I misunderstood how the config files are "inherited".

I will change the logic to what you proposed in 1 and 2 👍🏻

Ok, now the typo can be ignored in two places:

in the project's configuration file

in the explicit configuration file

However, it looks like if I remove the project's configuration file, the root is resolved to my home directory. This causes "ignore foo in the project" to add the ignore to that file instead.

🤔 I'm not sure what causes this. I do have that file in my home directory, so maybe this is some weirdness in the config resolution logic.

Any idea what could be a more reliable way to resolve the project directory?

Are you using nvim-lspconfig? It's root_dir logic is:

root_dir = util.root_pattern('typos.toml', '_typos.toml', '.typos.toml'),

ie: it will include ancestors in its search for config files

tekumara · 2024-06-16T07:44:37Z

crates/typos-lsp/src/typos/config_file_location.rs

+ }
+ }
+
+ Err(anyhow::anyhow!(


FYI - I've been using anyhow elsewhere in the project and then handling it in the LanguageServer impl like this - which seems to be working so far ...

Ok, on a quick glance that looks like anyhow can be used fine in these "worker" functions, and then unwrapped to comply with the return type expected by the language server implementation. I'll see if I can do the same, this looks quite good.

Did some improvement here, and it works. It also enabled some additional testing since the different concerns are no longer so tightly bound together.

Would you prefer more integration testing? Didn't look into it that much yet.

Nice one thank you - and yes it would be great to an integration test cover the execute command logic, if that's not too much trouble 🙏

tekumara · 2024-06-16T07:53:28Z

crates/typos-lsp/src/lsp.rs

+ };
+ }
+
+ Ok(None)


FYI probably the easiest way to trigger reloading the config file would be to call update_router which resets the router with new Instances (this approach does have a memory leak but that could be solved independently see #11)

This looks really good, thanks!

Reloading seems to work for me now!

tekumara · 2024-06-16T08:24:47Z

crates/typos-lsp/src/lsp.rs

+
+ std::fs::write(
+ &config_file_path,
+ toml::to_string_pretty(&config).expect("cannot serialize config"),


this appears to rewrite the entire file, removing comments and inserting empty defaults, eg:

[default] extend-ignore-identifiers-re = [] extend-ignore-words-re = [] extend-ignore-re = []

could we do a more surgical update, that only modifies default.extend-words and preserves comments ... perhaps toml_edit would help?

Nice idea, will definitely do this. I didn't realize comments get removed too, that's not good 😄

toml_edit worked nicely, and now the config file is preserved when ignoring a typo.

Chaitanyabsprip · 2024-09-10T09:35:48Z

any update/eta on this?

tekumara#12

mikavilpas · 2024-09-10T13:36:17Z

I kinda forgot about this for a long time... But I think I could still finish it.

To do this, I think the following can be worked on:

There are some open comment threads above that need to be solved
Additional testing would be welcomed. @Chaitanyabsprip if you have the time, let me know and I can explain how I tested this

mikavilpas · 2024-09-10T13:36:34Z

Also rebased to the latest main branch now.

tekumara · 2024-09-11T06:40:01Z

My apologies I missed the unanswered questions 🤦 and thank you @mikavilpas for offering to finish this! would be great to get this merged

Chaitanyabsprip · 2024-09-11T21:01:01Z

I will help you test this PR. I would require help in setting it up with my neovim configuration

mikavilpas marked this pull request as draft June 12, 2024 19:26

tekumara force-pushed the main branch 3 times, most recently from 29703cd to 6111f00 Compare June 16, 2024 05:10

tekumara reviewed Jun 16, 2024

View reviewed changes

mikavilpas force-pushed the add-misspelling-to-config-file branch 3 times, most recently from 5e8403a to ef9a6ad Compare June 21, 2024 10:04

mikavilpas added 2 commits September 10, 2024 16:25

feat: code action to add a misspelling to the config file

d31a079

tekumara#12

fixup! feat: code action to add a misspelling to the config file

9872ae8

mikavilpas force-pushed the add-misspelling-to-config-file branch from ef9a6ad to 9872ae8 Compare September 10, 2024 13:32

bbb651 mentioned this pull request Oct 18, 2024

Spell Checking zed-industries/zed#5305

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: feat: code action to add a misspelling to the config file #72

WIP: feat: code action to add a misspelling to the config file #72

mikavilpas commented Jun 12, 2024

tekumara left a comment

tekumara Jun 16, 2024 •

edited

Loading

mikavilpas Jun 17, 2024

mikavilpas Jun 21, 2024

tekumara Sep 11, 2024

tekumara Jun 16, 2024

mikavilpas Jun 17, 2024

mikavilpas Jun 21, 2024

tekumara Sep 11, 2024

tekumara Jun 16, 2024

mikavilpas Jun 17, 2024

mikavilpas Jun 21, 2024

tekumara Jun 16, 2024

mikavilpas Jun 17, 2024

mikavilpas Jun 21, 2024

Chaitanyabsprip commented Sep 10, 2024

mikavilpas commented Sep 10, 2024

mikavilpas commented Sep 10, 2024

tekumara commented Sep 11, 2024 •

edited

Loading

Chaitanyabsprip commented Sep 11, 2024

WIP: feat: code action to add a misspelling to the config file #72

Are you sure you want to change the base?

WIP: feat: code action to add a misspelling to the config file #72

Conversation

mikavilpas commented Jun 12, 2024

Requesting feedback

tekumara left a comment

Choose a reason for hiding this comment

tekumara Jun 16, 2024 • edited Loading

Choose a reason for hiding this comment

The way config resolution works is a bit gnarly.

Ok, so what?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Chaitanyabsprip commented Sep 10, 2024

mikavilpas commented Sep 10, 2024

mikavilpas commented Sep 10, 2024

tekumara commented Sep 11, 2024 • edited Loading

Chaitanyabsprip commented Sep 11, 2024

tekumara Jun 16, 2024 •

edited

Loading

tekumara commented Sep 11, 2024 •

edited

Loading