feat: add grammarly support

[mtoohey31]

Hello! This pull request adds a tiny config for the lsp version of grammarly. The original is built to work with a custom language client though, so I had to make some modifications to resolve errors that made it crash. The branch of my fork with those modifications is here, and there's instructions for building it in the docs for the configuration. There are still some references to features provided by the custom client in my fork, but the basic functionality of displaying diagnostics and applying simple text edit code actions works without errors. The use of some code actions might result in errors though, I haven't run into any yet but I have not done extensive testing. The modifications I made also removed the support for premium accounts, which I think is fine since, as far as I know, there's no way to handle the login prompt through the LSP alone without turning this into an extension.

Of note: this uses the Grammarly API, so every document that you open with this LSP running is seen by Grammarly. Would you like me to add a warning about this in the configuration docs?

Oh, and one last thing, the CONTRIBUTING.md file said not to use vim.fn.cwd, so I have the git ancestor helper in there right now, as far as my testing indicates though, the server does not care what the root dir is, even a function that always returns "." seems to work. I'd prefer to use something other than the git ancestor helper so that users aren't limited to using it in a git repository. If anyone has recommendations, let me know!

Thanks!

[mjlbach]

Usually we put the link to the language server as the first part of the description. "an LSP" dosen't make sense if you expand out the acronym (an language server protocol)? "A language server which provides access to Grammarly analysis via the API"

[mtoohey31]

Ok, I switched to your description and added the link in 357fa62. The link I gave was the one to the upstream repo though, does that make sense to you also, or would you prefer I make it the link to the Grammarly website or to my fork to avoid confusion?

[mjlbach]

You should link to upstream. Also LSP is the protocol, this is a language server.

[mjlbach]

Step 1 is still extremely unclear to me, please clarify the exact additional features afforded by your branch. It's also a long line, break it into multiple lines.

[mtoohey31]

You should link to upstream. Also LSP is the protocol, this is a language server.

Ah, my bad, it seems I have been misusing that acroymn. And also, sorry, I missed that your prior comment provided a description, I swapped it out for that one.

[mtoohey31]

Step 1 is still extremely unclear to me, please clarify the exact additional features afforded by your branch. It's also a long line, break it into multiple lines.

Ok, let me know if 3c11852 clears things up!

[kylo252]

Isn't it easier to use this https://github.com/emacs-grammarly/unofficial-grammarly-language-server? it seems to have an npm installation, and you can derive the settings from https://github.com/emacs-grammarly/lsp-grammarly

[mtoohey31]

Isn't it easier to use this https://github.com/emacs-grammarly/unofficial-grammarly-language-server? it seems to have an npm installation, and you can derive the settings from https://github.com/emacs-grammarly/lsp-grammarly

Thanks for bringing that to my attention! I took a look, and it seems that it requires the custom handlers too, but after thinking about it for a bit, it's probably best to put in the work and support those handlers anyway, as people may want to use those features and spending the time to maintain my fork doesn't seem realistic.

That'll take me a bit of time though, so I've converted this to a draft for now. I have my last exam tomorrow, so I'll have some free time to work on it this weekend hopefully.

[mjlbach]

I personally don't care which fork makes it into lspconfig's documentation. I assume the requests are standard between the forks between the two (following the protocol). I won't merge off-spec requests anyways (those belong in a separate plugin from lspconfig).

[mtoohey31]

Sounds good, I lean towards using the emacs fork since it appears very similar but is available through npm so that should simplify installation. When you refer to off-spec requests, do you mean that any requests with names not found in the official spec should be a separate plugin instead of being included here?

[mjlbach]

When you refer to off-spec requests, do you mean that any requests with names not found in the official spec should be a separate plugin instead of being included here?

Yes. if it's 1-2 off-spec requests I've been lenient, but generally if it require a lot of custom logic (deno is pretty borderline right now) it should live in its own plugin.

[mtoohey31]

Got it, I'll figure out which handlers are necessary and if it looks like a significant amount I'll go the plugin route.

[mjlbach]

It's also fine to add grammarly with some basic support here, and then in the server description/wiki direct people to a language specific plugin.

[mtoohey31]

Ok, I'll do that if I can too, it might not be possible since the server crashes completely if the requests I removed in my fork aren't handled, there were only a few that I've found so far though, so it should be doable.

[mjlbach]

To clarify, I do not care if the server internally does things "off-spec"/listens for off-spec requests, I only care if the size of server_configurations/grammarly starts to bloat because it adds many mapped commands that provide off-spec requests.

[mtoohey31]

Yep, I understand, I just mean that if there are more off-spec requests which the server makes and crashes if they are not handled, I might not be able to make the config usable without a bloated number of custom handlers. I think there are only one or two that are required though, so it should be fine!

[mtoohey31]

Ok, the latest commit switches things over to the emacs fork, and after much experimentation, the server works without crashing with only the $/updateDocumentState handler set to return an empty string.

The only thing that I wish I could do something about, but I don't think I can, is that the "Ignore Grammarly issue" code actions don't have any effect. Is that an acceptable shortcoming?

The only other remaining concerns on my end are the root_dir function and whether I should add a privacy notice to the docs.

[mjlbach]

The only thing that I wish I could do something about, but I don't think I can, is that the "Ignore Grammarly issue" code actions don't have any effect. Is that an acceptable shortcoming?

You could add a custom handler which filters this out. Do the other code actions work? If not, you can disable the capability for the server.

The only other remaining concerns on my end are the root_dir function and whether I should add a privacy notice to the docs.

The root dir seems fine, you can also try with single_file_support = true to allow launching on bare files.

I would add a privacy notice about grammarly. I don't use the service but I imagine their privacy policy is relatively draconian.

[mtoohey31]

You could add a custom handler which filters this out. Do the other code actions work? If not, you can disable the capability for the server.

Yes, the other actions work. I was attempting to figure out how to do that, but I think there's a gap in my understanding. I was searching through the data available to the handlers defined in the handlers property of the config, and the name of that particular code action never appears. I could be misunderstanding the way things work, but I believe code actions are communicated to the client in response to a codeAction request from the client to the server. If it's possible, would you be able to give me some pointers on how I might intercept the results from that and filter things?

The root dir seems fine, you can also try with single_file_support = true to allow launching on bare files.

I would add a privacy notice about grammarly. I don't use the service but I imagine their privacy policy is relatively draconian.

Will do, on both counts.

[mjlbach]

Actually, I would just ignore the code actions issue for now. It's a bit annoying to handle, since you can't just override the handler (only) like with some other requests.

[mjlbach]

LMK when this is ready for review, it seems fine to me now.

[mtoohey31]

Ok, yeah I think this is good to go now!

I'll mention this here in case anyone is considering writing a plugin and comes across this PR, some nice features that could be added as a plugin are:

• A function to display the current state that could be used in a statusline.
• The ability to change the document mode.
• Premium account login support.
• Proper support for $/getDocumentState and $/updateDocumentState, which should allow the "ignore" actions to work properly.

...and probably more that I haven't thought of. I don't really need those features personally though, so I don't plan on writing one right now.