antonWetzel / typst-languagetool

LanguageTool Integration for Typst for spell and grammer check
MIT License
36 stars 9 forks source link

Add LSP server's ability to send all hints/mistakes/notes to the client (like ltex) #7

Closed Andrew15-5 closed 3 months ago

Andrew15-5 commented 7 months ago

Hi. I was tired of using ltex all the time. And then people recommended me to check out this project. I assumed it would be like ltex, but more modern, Typst-only and Typst's specific keywords and syntax stuff removed from the list of mistakes. And I guess it does remove some false-positive errors, but I only was able to run it in a separate terminal. I'm using Neovim with cmp and lspconfig.

Is it in plans to be able to show all the messages right in the text editor? I would love to switch to this tool, because the non-real errors highlighting becomes really annoying at times and I have to remove the ltex server altogether.

antonWetzel commented 7 months ago

I use a VSCodium problem matcher to transform the plain output

START
<full file path> <line_start>:<column_start>-<line_end>:<column_end> info <rule> (<suggestions>)
...
END

into in editor suggestions. No clue if Neovim supports something like that.

The "correct" way is to implement the LSP-Protocol, but I have not done this before. I think LSP support and whatever the IDEs need makes sense, so I will look into this.

antonWetzel commented 7 months ago

I implemented a LSP with

I hope this works for Neovim. I used the first solution I found, so maybe some other events are better suited.

Andrew15-5 commented 7 months ago

image

image

This actually works! Wow! This is truly a breath of fresh air.

Though, there are some problems. When I tried code action, it not only showed fewer options (can't ignore/hide this specific error), but it also appended the fix instead of replacing:

ltex ![image](https://github.com/antonWetzel/typst-languagetool/assets/37143421/f7339962-4ecd-4999-8dd5-6148842277b0) ![image](https://github.com/antonWetzel/typst-languagetool/assets/37143421/ca107a7a-7ae0-46f5-a40d-3e73c7befbdf)
typst-lt ![image](https://github.com/antonWetzel/typst-languagetool/assets/37143421/a9ce1313-c8a8-4be1-8e0c-18fb5b76e209) ![image](https://github.com/antonWetzel/typst-languagetool/assets/37143421/79057098-33ce-407c-b1f4-e6e4dc307ce7)

Ok, I thought that it doesn't underline the things, but it actually because I had spell enabled:

image


The immediate problem is that I have to spin up languagetool server separately from LSP server. I guess ltex just is a wrapper for languagetool, because it instead starts the ltex-ls which is initially a shell wrapper for the java process. And if I automatically run the languagetool via lspconfig, then the client will stop with the error of not being able to connect to the server.

We have several solutions. User-side solution would be to make a shell wrapper for the typst-languagetool-lsp with a sleep 2. Then it should be enough for the server to start, but it is time-based, which is not good.

Then comes the dev-side solution. Which can be different. For example, you can run a subprocess via Command::new() which will run the server by itself. And also probably have to add an option that will get the languagetool-server.jar's path.

There are probably many more other options, but, of course, making life easier for the user is better, so I would suggest the dev-side solution above.

Andrew15-5 commented 7 months ago

I tried just waiting for the second server to start and it works perfectly fine:

    let client = ServerClient::new(&options.host, &options.port);
    dbg!("created a client");
    if client.ping().await.is_err() {
        eprintln!("Couldn't ping the LanguageTool server. Did you start it?");
        eprintln!("Waiting for the server to respond...");
        while client.ping().await.is_err() {
            sleep(Duration::from_millis(100));
        };
    }

Which is a good local temporary fix. You can also do this, but this means that the user would have to manage 2 servers themselves. I think calling the java yourself would be a better approach. The problem is that you can't make it a single file binary, because even ltex-ls ships a whole archive: https://github.com/valentjn/ltex-ls/releases/tag/16.0.0.

Maybe you can add both: either wait for the server to be started by the user, or try to start it yourself. I guess you can make the default one is to wait, and if the user set the path to the LanguageTool server jar, then you can try starting it yourself.

antonWetzel commented 7 months ago

I am working on a server_path option, which if provided does the java <server_path>.server.jar ... call and manages the server. The user needs to download the server and java, but not start the languagetool-server manually.

Andrew15-5 commented 7 months ago

You should add a description that the server path is for the LanguagTool server, because without the description it is weird that LSP server binary has a server_path option.

Andrew15-5 commented 7 months ago

I also noticed that you have a typo at:

https://github.com/antonWetzel/typst-languagetool/blob/2342bece0a1a8785b86bdd0cf40556b762067dcf/lsp/src/main.rs#L91

antonWetzel commented 7 months ago

I added a local_languagetool_folder: Option<PathBuf> option, which tries to start the server. Maybe just a windows thing, but the languagetool-server survives an process::Child::kill() and closing the lsp-server.

antonWetzel commented 7 months ago

Though, there are some problems. When I tried code action, it not only showed fewer options (can't ignore/hide this specific error), but it also appended the fix instead of replacing:

With VSCodium I see all replacements and the action replaces the word. image If Neovim acts different I have no clue how to fix it.

Andrew15-5 commented 7 months ago

It would be a great idea to add the language configuration. Either via CLI option or as an LSP server. For example, ltex-ls can have this settings:

  settings = {
    ltex = {
      language = "en-US",
    },
  },

But having a CLI options isn't great, at least for the established LSP servers, that are in the nvim-lspconfig list (https://github.com/neovim/nvim-lspconfig/blob/master/lua/lspconfig/server_configurations/typst_lsp.lua). The reason is that they would have a set of default settings, including the command, so that when you actually use the LSP server, you only would have to add a few things:

lspconfig.typst_lsp.setup {
  on_attach = on_attach,
  capabilities = capabilities,
  root_dir = function()
    return vim.fn.getcwd()
  end,
  settings = {
    exportPdf = "never",
  },
}

So adding an LSP setting would be ideal. But language probably isn't the only thing that should be added. At least from the CLI there are rules and dictionary file paths. Running the LanguageTool command will give even more options to configure:

LanguageTool server help message ```sh java -cp ~/.local/share/LanguageTool/languagetool-server.jar org.languagetool.server.HTTPServer --help ``` ``` Usage: HTTPServer [--config propertyFile] [--port|-p port] [--public] --config FILE a Java property file (one key=value entry per line) with values for: 'maxTextLength' - maximum text length, longer texts will cause an error (optional) 'maxTextHardLength' - maximum text length, applies even to users with a special secret 'token' parameter (optional) 'maxCheckTimeMillis' - maximum time in milliseconds allowed per check (optional) 'maxErrorsPerWordRate' - checking will stop with error if there are more rules matches per word (optional) 'maxSpellingSuggestions' - only this many spelling errors will have suggestions for performance reasons (optional, affects Hunspell-based languages only) 'maxCheckThreads' - maximum number of threads working in parallel (optional) 'cacheSize' - size of internal cache in number of sentences (optional, default: 0) 'cacheTTLSeconds' - how many seconds sentences are kept in cache (optional, default: 300 if 'cacheSize' is set) 'requestLimit' - maximum number of requests per requestLimitPeriodInSeconds (optional) 'requestLimitInBytes' - maximum aggregated size of requests per requestLimitPeriodInSeconds (optional) 'timeoutRequestLimit' - maximum number of timeout request (optional) 'requestLimitPeriodInSeconds' - time period to which requestLimit and timeoutRequestLimit applies (optional) 'languageModel' - a directory with '1grams', '2grams', '3grams' sub directories per language which contain a Lucene index each with ngram occurrence counts; activates the confusion rule if supported (optional) 'fasttextModel' - a model file for better language detection (optional), see https://fasttext.cc/docs/en/language-identification.html 'fasttextBinary' - compiled fasttext executable for language detection (optional), see https://fasttext.cc/docs/en/support.html 'maxWorkQueueSize' - reject request if request queue gets larger than this (optional) 'rulesFile' - a file containing rules configuration, such as .langugagetool.cfg (optional) 'blockedReferrers' - a comma-separated list of HTTP referrers (and 'Origin' headers) that are blocked and will not be served (optional) 'premiumOnly' - activate only the premium rules (optional) 'disabledRuleIds' - a comma-separated list of rule ids that are turned off for this server (optional) 'pipelineCaching' - set to 'true' to enable caching of internal pipelines to improve performance 'maxPipelinePoolSize' - cache size if 'pipelineCaching' is set 'pipelineExpireTimeInSeconds' - time after which pipeline cache items expire 'pipelinePrewarming' - set to 'true' to fill pipeline cache on start (can slow down start a lot) Spellcheck-only languages: You can add simple spellcheck-only support for languages that LT doesn't support by defining two optional properties: 'lang-xx' - set name of the language, use language code instead of 'xx', e.g. lang-tr=Turkish 'lang-xx-dictPath' - absolute path to the hunspell .dic file, use language code instead of 'xx', e.g. lang-tr-dictPath=/path/to/tr.dic. Note that the same directory also needs to contain a common_words.txt file with the most common 10,000 words (used for better language detection) --port, -p PRT port to bind to, defaults to 8081 if not specified --public allow this server process to be connected from anywhere; if not set, it can only be connected from the computer it was started on --allow-origin [ORIGIN] set the Access-Control-Allow-Origin header in the HTTP response, used for direct (non-proxy) JavaScript-based access from browsers. Example: --allow-origin "https://my-website.org" Don't set a parameter for `*`, i.e. access from all websites. --verbose, -v in case of exceptions, log the input text (up to 500 characters) --languageModel a directory with '1grams', '2grams', '3grams' sub directories (per language) which contain a Lucene index (optional, overwrites 'languageModel' parameter in properties files) --premiumAlways activate the premium rules even when user has no username/password - useful for API servers ```

Speaking of great ideas, I noticed that you can provide a language directly in the HTTP request:

    for items in data {
        let req = CheckRequest::default()
            .with_language(match language {
                Some(value) => String::from(value),
                None => "auto".into(),
            })
            .with_data(Data::from_iter(items.0));

        let mut response = client.check(&req).await?;

        filter_response(&mut response, dict);
        action(response, items.1);
    }

And so I thought that it would be very cool if I can finally get suggestions for more than 1 language, because ltex-ls only works with a single language AFAIK and to add a second you would have to create another config copy of that LSP server, change the language and start the second server. And since there is no point in running 2 servers to just call with a different language, instead we can spin up only a single one, and if there are more than 1 language selected, then loop through them and add diagnostics for each language. I just tried 2 ltex-ls server in parallel for different languages — they show diagnostics for different languages ok, but for the Typst syntax they duplicate/stack the errors. With this LSP server it shouldn't be an issue, since it filters those.

Andrew15-5 commented 7 months ago

Oh, I see you already added some this...

Andrew15-5 commented 7 months ago

Hmm, I thought I was using my custom version, because it behaves like one. But then the LT server didn't start, and I went to the logs. And logs were filled with Waiting for the server to respond.... I thought, "I couldn't have started the server that many times." Apparently my code was used, and the message was added in the loop as well:

https://github.com/antonWetzel/typst-languagetool/blob/3eb4ead9df72be04f7b797f2c5e5588441975b78/lsp/src/main.rs#L144-L153

I think you should remove the message from the loop, because it doesn't help at all, it only quickly increases the log file size.

Andrew15-5 commented 7 months ago

Do I need to wrap my settings in another object? Can you give your LSP config?

antonWetzel commented 7 months ago

For VSCode I use

// settings.json
{
    //...
    "generic-lsp.configuration": [
        {
            "language": "typst",
            "lsp": "typst-languagetool-lsp",
            "options": {
                "language": "de-DE",
                "dictionary": [
                    "Normalen",
                    "Voronoi-Diagramm",
                    "LASzip",
                    "LASzip-Format",
                    "Octree",
                    "Quadtree",
                ],
                "local_languagetool_folder": "D:/LanguageTool-6.3/",
            }
        }
    ],
    //...

The options object is used in the js/node extension

const clientOptions: LanguageClientOptions = {
    initializationOptions: options,
    documentSelector: [{ scheme: 'file', language: language }],
};

In the init call the settings are passed as one object in the initializationOptions field.

Andrew15-5 commented 7 months ago

There is a chance that you are retrieving configuration incorrectly.

Here is how config options are retrieved in tinymist:

code ```rs fn did_change_configuration(&mut self, params: DidChangeConfigurationParams) -> LspResult<()> { // For some clients, we don't get the actual changed configuration and need to // poll for it https://github.com/microsoft/language-server-protocol/issues/676 match params.settings { JsonValue::Object(settings) => self.on_changed_configuration(settings)?, _ => { self.client.send_request::( ConfigurationParams { items: Config::get_items(), }, |this, resp| { if let Some(err) = resp.error { log::error!("failed to request configuration: {err:?}"); return; } // .map(Config::values_to_map), let Some(result) = resp.result else { log::error!("no configuration returned"); return; }; let resp: Vec = serde_json::from_value(result).unwrap(); let _ = this.on_changed_configuration(Config::values_to_map(resp)); }, ); } }; Ok(()) } ```

And here is typst-lsp example:

code ```rs async fn did_change_configuration(&self, params: DidChangeConfigurationParams) { // For some clients, we don't get the actual changed configuration and need to poll for it // https://github.com/microsoft/language-server-protocol/issues/676 let values = match params.settings { JsonValue::Object(settings) => Ok(settings), _ => self .client .configuration(Config::get_items()) .await .map(Config::values_to_map), }; let result = match values { Ok(values) => { let mut config = self.config.write().await; config.update_by_map(&values).await } Err(err) => Err(err.into()), }; match result { Ok(()) => { info!("new settings applied"); } Err(err) => { error!(%err, "error applying new settings"); } } } pub async fn configuration( &self, items: Vec, ) -> jsonrpc::Result> { self.send_request::(ConfigurationParams { items }) .await } ```

AFAICT, this function isn't called anywhere, instead this command is sent to the LSP server by the client and the client itself gives the config. But I have no idea how LSP works specifically, so maybe it's a bit different.

I tried debugging typst-lsp, and firstly it has different config and the initialization_options property is an empty object which is identical to the typst-languagetool-lsp's output.

antonWetzel commented 7 months ago

(based on my limited knowledge) There are 2 ways to get config from the ide to the lsp. On init https://neovim.io/doc/user/lsp.html#vim.lsp.ClientConfig in the init_options or at runtime by a config changed notificaton (ide->lsp), config request (lsp->ide) and config response (ide->lsp). The config changed notificaton may contain the config, so the follow up request is not required.

VSCodium has no native LSP support and I could get the config request to work with my vscodium extension. So at the moment only the init is supported. The examples look like the runtime variant, which should also be supported.

Andrew15-5 commented 7 months ago

Ok. Thanks for the tip. This indeed now finally works:

lsp_configs.typst_languagetool = {
  default_config = {
    cmd = { vim.env.HOME .. "/.local/share/cargo/bin/typst-languagetool-lsp" },
    filetypes = { "typst" },
    root_dir = function()
      return vim.fn.getcwd()
    end,
  },
}

lspconfig.typst_languagetool.setup {
  on_attach = on_attach,
  capabilities = capabilities,
  init_options = {
    local_languagetool_folder = vim.env.HOME .. "/.local/share/LanguageTool/",
  },
}
Andrew15-5 commented 7 months ago

The first problem, aside from needing to use init_options instead of settings, is that the local_languagetool_folder (Window$ user detected; please consider renaming folder to dir) must end with a slash, because in the code:

    let _server = if let Some(path) = &options.local_languagetool_folder {
        let mut jar = path.clone();
        jar.push("languagetool-server.jar");

which completely ignores the fact that without the slash it will just concatenate the dir name with the file name, which is of course will be invalid. A fix is to use join() instead:

    let _server = if let Some(path) = &options.local_languagetool_folder {
        let jar = path.clone().join("languagetool-server.jar");

The second problem is that the original command didn't have the:

            .arg("--config")
            .arg(config)

Which is why I had an error which looked for this file:

        let mut config = path.clone();
        config.push("server.properties");

that is of course doesn't exist. You should instead add this thing to the options:

struct Options {
    language:                  Option<String>,
    host:                      String,
    port:                      String,
    request_length:            usize,
    rules:                     Rules,
    dictionary:                HashSet<String>,
    local_languagetool_folder: Option<PathBuf>,
}

Maybe an Option<PathBuf> (default is None)? Is it for the LanguageTool server's options? I didn't set them with ltex-ls, so I don't want to use them forcefully. At least make it opt-out, but I would insist on opt-in.

Alright. I think that's it for immediate issues. Oh, wait, no, one more thing — you locked in the rustfmt version which prevents me from using mine:

Failed to run formatter rustfmt. Error writing files: version mismatch

Removing this line:

required_version = "1.5.2"

fixes the problem. Also now a bunch of things are formatted. I'm using rustfmt 1.7.0-nightly (f704f3b 2023-12-19).

Performs the git pull. I see you removed the repetitive waiting log message, which I appreciate. Oh, btw, I noticed one seemingly harmless error in my logs:

logs I added a bit of random formatting: ```sh tail -n 110 lsp.log | sed -E 's/.*stderr"\s+//' | tr -d '\n' | sd '\\n' '\n' ``` ```log ... '"starting client at http://127.0.0.1:8081 ""Waiting for the server to respond. ""2024-03-14 04:16:05.511 GMT+03:00 INFO org.languagetool.server.DatabaseAccessOpenSource Not setting up database access, dbDriver is not configured ""2024-03-14 01:16:05 +0000 Setting up thread pool with 10 threads ""WARN: no common words file defined for Japanese - this language might not be correctly auto-detected ""WARN: no common words file defined for Khmer - this language might not be correctly auto-detected ""2024-03-14 04:16:06.367 GMT+03:00 INFO org.languagetool.server.TextChecker A/B-Test enabled: [] ""2024-03-14 01:16:06 +0000 Starting LanguageTool 6.3 (build date: 2023-12-11 12:54:40 +0000, de815a0) server on http://localhost:8081... ""2024-03-14 01:16:06 +0000 Server started ""2024-03-14 04:16:06.504 GMT+03:00 INFO org.languagetool.server.LanguageToolHttpHandler Handling GET /v2 ""2024-03-14 04:16:06.509 GMT+03:00 INFO org.languagetool.server.LanguageToolHttpHandler An error has occurred: 'This is the LanguageTool API. You have not specified any parameters. Please see https://languagetool.org/http-api/swagger-ui/#/default', sending HTTP code 400. Access from 127.0.0.1, HTTP user agent: null, User agent param: null, Referrer: null, language: null, h: 1, r: 1, time: 14m: ALL, l: DEFAULT, Stacktrace follows:org.languagetool.server.BadRequestException: This is the LanguageTool API. You have not specified any parameters. Please see https://languagetool.org/http-api/swagger-ui/#/default at org.languagetool.server.LanguageToolHttpHandler.handle(LanguageToolHttpHandler.java:212) at jdk.httpserver/com.sun.net.httpserver.Filter$Chain.doFilter(Filter.java:98) at jdk.httpserver/sun.net.httpserver.AuthFilter.doFilter(AuthFilter.java:82) at jdk.httpserver/com.sun.net.httpserver.Filter$Chain.doFilter(Filter.java:101) at jdk.httpserver/sun.net.httpserver.ServerImpl$Exchange$LinkHandler.handle(ServerImpl.java:871) at jdk.httpserver/com.sun.net.httpserver.Filter$Chain.doFilter(Filter.java:98) at jdk.httpserver/sun.net.httpserver.ServerImpl$Exchange.run(ServerImpl.java:847) at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1144) at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:642) at java.base/java.lang.Thread.run(Thread.java:1583) ""2024-03-14 04:16:06.537 GMT+03:00 INFO org.languagetool.server.LanguageToolHttpHandler Handled request in 44ms; sending code 400 ""2024-03-14 04:16:06.537 GMT+03:00 INFO org.languagetool.server.LanguageToolHttpHandler Handling POST /v2/check ... ```

I tried it with multiple languages and default settings, and it looks like it has a language priority. But if I remove a mistake from a "priority language" it will instead show mistakes from another language. Previously, I mentioned the desire to be able to set multiple languages. I'm not sure if you are on the board with this yet. Can you please add a license to your repo? I personally use (A)GPL, others like Apache/MIT. I want to fork your repo and play a bit with this "multi-language" feature.

Andrew15-5 commented 7 months ago

Oh, one more idea came to my mind. If the user has the same LanguageTool server settings, which means that its CLI command string is identical every time, then you can check if the server already exists before making a new one. You can call it like reuse_languagetool_server which can be set to either one (true/false) by default. I just don't know which one is better. So the result is if I'm working on multiple projects/files (in different instances of the editor), then the LSP server will have multiple instances running simultaneously, but all of them will use the same LT server which is obviously will run on the same port. Right now this is already kinda works, because when the second LSP server is run, it will try to run the new java command which will fail, but the result of _server isn't checked, so even though the (new/second) server has failed to run (due to port being occupied), the previous server is already running, so instead it uses the previous one (I assume) unintentionally.

antonWetzel commented 7 months ago

The first problem, aside from needing to use init_options instead of settings

The LSP listens to the DidChangeConfiguration notification, but expects the data in the notifiction.

please consider renaming folder to dir

Changed to directory (Windows + VSCodium + Fira Code => no personality here).

without the slash it will just concatenate the dir name with the file name

join is the better method, but push should work. https://doc.rust-lang.org/stable/std/path/struct.PathBuf.html#method.push image

Which is why I had an error which looked for this [.../server.properties] file:

I added a check if the config file exists, but not an option to specify the location.

you locked in the rustfmt version which prevents me from using mine

The rustfmt.toml was copied from an older project with nightly, I removed all nightly options so stable should work.

noticed one seemingly harmless error in my logs

I think the error is from the Ping to check if the server exists

Can you please add a license to your repo

I added the MIT license.

Checking / Reusing the languagetool Server

The try to start and then try to connect method is pretty hacky, but does work. I think most (all) users want to use the exisiting languagetool server if it is already running, even if the options say start a new one.

antonWetzel commented 7 months ago

But if I remove a mistake from a "priority language" it will instead show mistakes from another language

I always specify the language code, but the languagetool server should detect the language and only send results for the found language.

Multi language projects

At the moment the language option (code or auto) is specified on LSP/Project level. I think a finer control is good and I see 2 options

I think (1) can work cleanly with directory or glob support, the settings would maybe look like this

"language-overwrite": {
   "paper/": "en-CA",
   "arbeit/": "de-DE",
   "readme.typ": "en-US",
}

(2) would look maybe like this

// LT:language=en-CA
Some english text to check.
// LT:language=de-DE
Aus irgendeinem Grund muss der zweite Teil in Deutsch sein.

Mix of languages

I think the Languagetool api does not allow multiple languages for the same text.

Andrew15-5 commented 7 months ago

The LSP listens to the DidChangeConfiguration notification, but expects the data in the notifiction.

I don't understand what you are trying to say. I checked the "usual" way of specifying settings instead of init_options — it doesn't work. So whatever the LSP server does, it does it not correctly or fully (from a user perspective). I'm not close to LSP (yet) to understand all the shenanigans that are set by Micro$oft and have to be done in order for settings to work.

Changed to directory (Windows + VSCodium + Fira Code => no personality here).

Thank you. ("no personality here"? I don't understand if this is a sarcasm or not, but I was referring to only Window$ having "folders" while all the UNIX systems (Linux, macOS, FreeBSD) having "directories", therefore by adding "folder" to the option's name you have exposed the "Window$ user" part of yourself. :) I also use Fira Code btw.)

join is the better method, but push should work.

Oh, right, sorry. I haven't read the docs, I just had a gut feeling that push will just append the string, which means it won't automatically add the directory separator symbol. But yeah, join is still better, and it is actually more common — I remember using similar join() in JS (and maybe in other languages as well).

I added a check if the config file exists, but not an option to specify the location.

Well, that is better, but if you want to keep it that way, you should probably mention this fact in the readme file, so that the user would know and use it, if desired so. Since I'm not that close with all the ins and outs of the LT server usage, I would assume that the settings file can actually be stored outside the LT server's directory, simply because it is not included in the distribution archive. It would be just a small improvement, IMO, if a custom path (including the file name, probably) is added.

I think the error is from the Ping to check if the server exists

Really? It also appears when starting the first LT server. Do you think that it prints the message even if you do not propagate the error?

async fn check_languagetool_server(client: &ServerClient) -> Result<(), Box<dyn Error>> {
    eprintln!("Waiting for the server to respond.");
    for _ in 0..(4 * 60) {
        if client.ping().await.is_ok() {
            return Ok(());
        }
        std::thread::sleep(std::time::Duration::from_millis(250));
    }
    client.ping().await?;
    Ok(())
}

I would assume calling the .is_ok() wouldn't print any error message.

I added the MIT license.

Thank you!

The try to start and then try to connect method is pretty hacky, but does work.

Yes.

I think most (all) users want to use the exisiting languagetool server if it is already running, even if the options say start a new one.

I don't 100% understand you. The idea of reusable LT server would probably appeal to most users. Which "options" do you refer to? I wanted to hear what is your opinion on slightly changing the logic, basically checking for the LT server before trying to create a new one. This way there won't be any unnecessary errors. You can throw in Checking/searching for running LanguageTool server or something before the initial ping. I mean, if all users are agreed on this behavior, then there is no point in creating a new option to do or do not reuse the LT server. But imagine if the user changed some settings, which would mean that a new server with different settings should be started. If instead the previous is used, then the new settings wouldn't apply. So in the end, providing the ability to take control over creation/reusage of the LT server can be beneficial to some folks.

Andrew15-5 commented 7 months ago

I think the Languagetool api does not allow multiple languages for the same text.

Which is exactly my point. We need to have some easy way of being able to check for mistakes in multiple languages. Because what you've described above means that at each point in any file, there would be only a single language active. I think you can imagine a use case where multiple languages can be used in a single file. Specifying the magic comment each time to change the language isn't very flexible. The simple example is if you have multiple languages in a single line, then it would be nice to know if there are any spelling mistakes in both languages on that single line. For me, I think that this is not very useful, as I tend to write in a single language throughout an entire document. But if I would write a comment about Typst stuff in English (because Typst uses English keywords/syntax) and then the document content in another language, then I would definitely be able to benefit from multi-language feature.

Going back to the "finer control is good and [...] 2 options", I can say that magic comment would definitely be more flexible than using some glob patterns in the LSP settings. But are there any problems if you just use multiple languages by default? Like why would you need to specify a separate language for a separate file/section, if the LT server would see in which language text is written and only show mistakes for where mistakes do exist regardless of the language.

I use Neovim's built-in feature of detecting (most) spelling mistakes, which actually supports multiple languages (I used it for a long time with only specifying a single language at a time). :h spelllang:

docs ```help *'spelllang'* *'spl'* 'spelllang' 'spl' string (default "en") local to buffer A comma-separated list of word list names. When the 'spell' option is on spellchecking will be done for these languages. Example: set spelllang=en_us,nl,medical This means US English, Dutch and medical words are recognized. Words that are not recognized will be highlighted. The word list name must consist of alphanumeric characters, a dash or an underscore. It should not include a comma or dot. Using a dash is recommended to separate the two letter language name from a specification. Thus "en-rare" is used for rare English words. A region name must come last and have the form "_xx", where "xx" is the two-letter, lower case region name. You can use more than one region by listing them: "en_us,en_ca" supports both US and Canadian English, but not words specific for Australia, New Zealand or Great Britain. (Note: currently en_au and en_nz dictionaries are older than en_ca, en_gb and en_us). If the name "cjk" is included East Asian characters are excluded from spell checking. This is useful when editing text that also has Asian words. Note that the "medical" dictionary does not exist, it is just an example of a longer name. *E757* As a special case the name of a .spl file can be given as-is. The first "_xx" in the name is removed and used as the region name (_xx is an underscore, two letters and followed by a non-letter). This is mainly for testing purposes. You must make sure the correct encoding is used, Vim doesn't check it. How the related spell files are found is explained here: |spell-load|. If the |spellfile.vim| plugin is active and you use a language name for which Vim cannot find the .spl file in 'runtimepath' the plugin will ask you if you want to download the file. After this option has been set successfully, Vim will source the files "spell/LANG.vim" in 'runtimepath'. "LANG" is the value of 'spelllang' up to the first character that is not an ASCII letter or number and not a dash. Also see |set-spc-auto|. ```

And this is why I am thinking about multi-language support for the typst-languagetool-lsp (or LT server, really). Because it will just detect a language automatically from a list of specified languages and then show mistakes for each language in the list. I think adding (advanced) grammar mistakes to the already mostly working spell checker would be awesome (:set spell by default supports things like "capitalize word at the beginning of a sentence").

Andrew15-5 commented 7 months ago

It looks like the code action still doesn't replace mistakes correctly (https://github.com/antonWetzel/typst-languagetool/issues/7#issuecomment-1992593290).

tingerrr commented 6 months ago

I've scanned over the issue quickly to see that it does indeed work with Andrew's neovim config.

I can't seem to get it to work with helix, when I initially downloaded it with remote-server support, it worked when running the cli directly. But it did not work within helix and the lsp did not shut down on time before helix pulled the plug 2 seconds later.

As I was writing this issue, I noticed that the cli no longer works either, it simply hangs with the same invocation as before.

I ran the cli today like so:

typst-languagetool check --host https://localhost --port 8081 --language de-DE src/de/thesis.typ

and configured the lsp like so:

[language-server.typst-languagetool]
command = "typst-languagetool-lsp"
config.language = "de-DE"
config.host = "https://localhost"
config.port = "8081"

[[language]]
name = "typst"
language-servers = ["typst-languagetool"]

edited or brevity.

I haven't noticed any logging settings, so I can only provide you the log messages helix gave me:

2024-04-17T15:15:40.471 helix_lsp::transport [ERROR] typst-languagetool err <- "starting LSP server\n"
...
2024-04-17T15:16:57.636 helix_lsp::transport [ERROR] typst-languagetool err: <- StreamClosed

EDIT: It turns out I was not using the exact same invocation, I somehow used https in the second time and in the config. It's interesting that it simply hangs there instead of failing to connect.

Andrew15-5 commented 6 months ago

AFAIK, you don't need to set host and port. So my advice was to remove all the custom settings and run it as it is, but you already fixed the problem.

Also, it's strange that helix doesn't give you all the logs, because in Neovim the "standard/default" logs are just coming directly from LSP servers. This server also prints the provided init settings, IIRC.

tingerrr commented 6 months ago

I get all logs sent by tinymist without problems. But it could be the log severity.

antonWetzel commented 5 months ago

I still don't know why quick fixes don't remove the diagnostic and update the range if text is inserted before.

With the on_change: <timeout> option text is checked on change after the timeout.

The outdated diagnostic should be removed after the timeout

antonWetzel commented 3 months ago

I am closing this, because the general LSP capabilities are implemented. For errors or further improvements, please open a new issue.