Refactor to remove Vale Server but add some features

[ChrisChinchilla]

Very much a draft…

But @jdkato I think this will require some changes to Vale CLI to work fully, so…

remove: easy, done.
replace: Looking at the JSON output, there's no actual parameter to send to the extension, so for example:

    {
      "Action": {
        "Name": "replace",
        "Params": null
      },
      "Check": "Openly.GenderBias",
      "Description": "",
      "Line": 9,
      "Link": "",
      "Message": "Consider using 'worker(s)' instead of 'workman'",
      "Severity": "suggestion",
      "Span": [
        9,
        15
      ],
      "Match": "workman"
    },

I can see the suggested replacement in Message, but that's not much use, ideally it should be in the Params part of Action, which I think comes from Vale CLI… I could parse the string and remove the suggestion etc, but that feels like a messy solution.

More to follow, but this is my main issue right now.

[lgtm-com]

This pull request introduces 3 alerts when merging 51875bc into feec982 - view on LGTM.com

new alerts:

• 3 for Unused variable, import, function or class

[ChrisChinchilla]

@jdkato I also wonder how many of the old Vale Server fixers are actually supported in Vale CLI?

	"suggest": suggest,
	"replace": replace,
	"remove":  remove,
	"convert": convert,
	"edit":    edit,

[apupier]

• I think the setting vale.core.useCLI should also be removed
• it implies to update some commands visibility condition or maybe event to remove them
• there are several commented-out lines of code. Cannot we removed them completely? or it is planned before merging this PR to rework them to have feature replacement? Even in this case, what do you think about doing it in another iteration?

[apupier]

the renaming of the VS code extension name have some implications:

• users won't be automatically updated to this version
• it is possible to mark the old one as deprecated and mention that the migration should go to this new one. For that, we will need to mention it on this Github discussion Deprecated extensions microsoft/vscode-discussions#1

[apupier]

in package-lock.json, teh name is still vale-server

[jdkato]

As we discussed when merging extensions, I don't think we should change this.

[ChrisChinchilla]

@jdkato Personally I think it's really confusing now when there is no such thing as Vale server to call the extension vale server, but to avoid further confusion to users, let's keep it as is for now… Maybe I'll see if there's some way to handle it.

[apupier]

I can see the suggested replacement in Message, but that's not much use, ideally it should be in the Params part of Action, which I think comes from Vale CLI… I could parse the string and remove the suggestion etc, but that feels like a messy solution.

agree. reported errata-ai/vale#462

[ChrisChinchilla]

Yes, this a draft pr, far from finished.

[jdkato]

... ideally it should be in the Params part of Action ...

This is indeed how it's supposed to work. It was broken (inadvertently) in a semi-recent commit, but it'll be fixed in the next release.

---

@apupier, please be mindful of the fact that I follow every repo in this organization. You don't have to open issues to tell me to look at other issues or create discussions asking about existing issues -- it just creates more emails for me to shift through.

[apupier]

@apupier, please be mindful of the fact that I follow every repo in this organization. You don't have to open issues to tell me to look at other issues or create discussions asking about existing issues -- it just creates more emails for me to shift through.

sorry. I was trying to do my best to report issues to the most accurate repository. I will do differently as I'm accustomed when contributing to this organization so that it fits better to you, the creator and main maintainer of it.

[jdkato]

@jdkato I also wonder how many of the old Vale Server fixers are actually supported in Vale CLI?

	"suggest": suggest,
	"replace": replace,
	"remove":  remove,
	"convert": convert,
	"edit":    edit,

The replace issue is fixed in the latest release. The others just pass the rule-defined params back out through the JSON output, so the CLI doesn't do anything special.

[ChrisChinchilla]

@jdkato We can handle some of the other discussion later, but now that fixed replace… Any idea about the spelling suggestions?

The JSON output just returns

      "Action": {
        "Name": "suggest",
        "Params": [
          "spellings"
        ]

So not quite sure what to do with that 😆

[lgtm-com]

This pull request introduces 3 alerts when merging 3ceee47 into bead57d - view on LGTM.com

new alerts:

• 3 for Unused variable, import, function or class

[jdkato]

@jdkato We can handle some of the other discussion later, but now that fixed replace… Any idea about the spelling suggestions?

The JSON output just returns

      "Action": {
        "Name": "suggest",
        "Params": [
          "spellings"
        ]

So not quite sure what to do with that 😆

This was all server-side before: the idea is that suggest tells you the action, spellings tells you the expected output, and Match gives you the word.

You can then use a spell-check library (or maybe VS Code's built-in one?) to provide spelling suggestions.

[ChrisChinchilla]

@jdkato Hmm, not sure there is an in-built one. This might take some extra work. So what I might do is polish and wrap up this PR and then think about that and add it later. One other question, does the check take into account the ignore words, or will VSCode also need to take into account those?

[jdkato]

One other question, does the check take into account the ignore words

Yes, since an alert will only be raised if Match wasn't ignored by the current Vale configuration.

Also, even if the action stuff isn't 100% implemented, I think the dep updates and doc changes would be good to get in soon.

[lgtm-com]

This pull request introduces 3 alerts when merging fce8f5c into bead57d - view on LGTM.com

new alerts:

• 2 for Unused variable, import, function or class
• 1 for Syntax error

[ChrisChinchilla]

@jdkato So spelling isn't implemented, but I think everything else is for now.

[ChrisChinchilla]

Possibly also other future features to return, will go through them slowly.

[lgtm-com]

This pull request introduces 2 alerts when merging 0836782 into bead57d - view on LGTM.com

new alerts:

• 2 for Unused variable, import, function or class

[jdkato]

This looks good to me.