errata-ai / vale

:pencil: A markup-aware linter for prose built with speed and extensibility in mind.
https://vale.sh
MIT License
4.3k stars 144 forks source link

Regex and Script actions do not provide suggestions #851

Closed Firstyear closed 2 weeks ago

Firstyear commented 2 weeks ago

Check for existing issues

Environment

brew install vale

# vale -v
vale version 3.5.0

Describe the bug / provide steps to reproduce it

When attempting to use basic actions to provide suggestions, no suggestions are made.

text.md

a thunderly-chicken comes

.vale.ini

StylesPath = styles
MinAlertLevel = suggestion

[*.md]
BasedOnStyles = kanivale

styles/kanivale/chicken.yml

extends: existence
message: "'%s' should be '%s'"
nonword: true
level: error
action:
  name: suggest
  params:
    - chicken.tengo
tokens:
  - 'chicken'

styles/config/actions/chicken.tengo

text := import("text")
suggestions = ["chimken"]

styles/kanivale/alt_chicken.yml

extends: existence
message: "'%s' should be '%s'"
nonword: true
level: error
action:
  name: suggest
  params:
    - regex
    - 'chicken'
    - 'chimken'
tokens:
  - 'chicken'

Actual Output

vale test.md

 8:13  error    'chicken' should be ''          kanivale.alt_chicken
 8:13  error    'chicken' should be ''          kanivale.chicken

✖ 2 errors, 0 warnings and 0 suggestions in 1 file.

Expected Output

vale test.md

 8:13  error    'chicken' should be 'chimken'          kanivale.alt_chicken
 8:13  error    'chicken' should be 'chimken'          kanivale.chicken

✖ 2 errors, 0 warnings and 0 suggestions in 1 file.

Extra Notes

While we know that it's possible to do basic replacement with other actions, I tried to reduce these to the most specific examples and cases. We would like regex rules to provide the output of their actions/suggestions to the user, but we have been unable to make this feature work.

Is there something we are missing?

alexvonme commented 2 weeks ago

styles/kanivale/alt_chicken.yml

extends: existence
message: "'%s' should be '%s'"
nonword: true
level: error
action:
  name: suggest
  params:
    - regex
    - 'chicken'
    - 'chimken'
tokens:
  - 'chicken'

We were discussing this with @Firstyear today, and his other scripts also had name: edit. Either way, we repeatedly failed to get the correct suggestions from vale regex option.

@ccoVeille have you ever used the regex option? Unlike you, I'm not comfortable with Golang. Could you help us here?

ccoVeille commented 2 weeks ago

I have never used regexp in a vale rule, but I can check the code for sure.

You provided a real life example.

ccoVeille commented 2 weeks ago

My guess is existence + edit + regexp is simply not able to use the third parameter as %s in message

alexvonme commented 2 weeks ago

I was trying to retrieve a replace like functionality where the rule also suggests the appropriate fix. However, I've not been able to produce the correct replacement suggestion, even though the token works appropriately and spots the problem phrase.

ccoVeille commented 2 weeks ago

There might be a bug a not, but there is something strange anyway.

The example you provide is maybe a minimal version of how to reproduce the bug you are facing. If so, then OK.

Otherwise, I wouldn't have used such a rule/setting

I mean I would have used substitution

https://vale.sh/docs/topics/styles/#substitution

This one is dedicated to what I understand you tried to achieve with your rules.

It supports regexp, and you can use the same rules for multiple tokens. Something I'm afraid you cannot with the rule you used and shared.

So let me know, but yes, I do think the code you shared should have provided the suggestion in the second %s

ccoVeille commented 2 weeks ago

You can have a look at the files involved in this PR

https://github.com/openSUSE/suse-vale-styleguide/pull/98

alexvonme commented 2 weeks ago

Thank you for confirming the issue. At this point, I'm only trying to understand how the regex option is supposed to work. It might be a very useful tool for certain edits.

In alternate cases, I agree that substitution is ideal since, as you pointed out, it allows for multiple tokens.

ccoVeille commented 2 weeks ago

You will have to wait for someone with more experience in the code base, maybe @jdkato

jdkato commented 2 weeks ago

Actions don't populate the message field; they provide instructions to external tools on how to edit the source file.

Your example would be best implemented using substitution:

extends: substitution
message: "Use '%s' instead of '%s'."
level: error
action:
  name: replace
swap:
  chicken: chimken

Then, for example, in an editor using vale-ls you'll see a 'Quick Fix' button that will perform the action:

Screenshot 2024-06-19 at 1 33 09 PM

An example using regex would be something like this:

extends: existence
message: "'%s' should be in kebab-case."
level: error
action:
  name: edit
  params:
    - regex
    - '(\w+)_(\w+)'
    - "$1-$2"
tokens:
  - '\w+_\w+'

Which would yield the following (dynamically updating to the instance the mouse is over):

Screenshot 2024-06-19 at 1 41 53 PM

Firstyear commented 2 weeks ago

Actions don't populate the message field; they provide instructions to external tools on how to edit the source file.

The problem is that this behaviour is inconsistent. some actions provide suggestions to a CLI user, and some do not.

Your example would be best implemented using substitution:

The example was an intentionally minimised reproducer that regex doesn't behave as expected - We have far more interesting regex use cases in mind.

extends: substitution
message: "Use '%s' instead of '%s'."
level: error
action:
  name: replace
swap:
  chicken: chimken

The thing here is that the "action" in the substitution on the cli does provide the suggestion. For example it would give output like the below.

vale test.md

 8:13  error    'chicken' should be 'chimken'          kanivale.chicken

✖ 1 errors, 0 warnings and 0 suggestions in 1 file.

To put this another way - is there a reason why the regex or script actions do not or can not provide suggestions on the vale cli? Without the ability to yield these on the cli, this artificially limits what rules can be created because there is insufficient feedback to the user.

An example is here:

https://github.com/errata-ai/Microsoft/blob/master/Microsoft/Hyphens.yml

This message on this rule shows message: "'%s' doesn't need a hyphen." when it could say message: "'%s' doesn't need a hyphen. Replace with %s".

This message only works because the replacement is trivial, but a more complex replacement would not be possible such as group match re-ordering.

Another example is the suggest example in the vale docs https://vale.sh/docs/topics/actions/#suggest

This example is a perfect use of regex or a script because it could automatically suggest on the CLI that the user replaces SomeCamelCaseText with snake_case_text instead. But instead, it can not do so because these actions don't propogate suggestions to the message. This leaves the message only as message: "'%s' should be in snake_case." when the message could show the replacement snake case version.

It's especially misleading as suggests as an action name should be able to indicate suggestions to the user on the cli, and this module is unable to do so.

To summarise - the bug is that regex and suggest actions do not propogate their actions into the message string stack for display when using the vale cli. This affects users who do not use the vale language server.

jdkato commented 2 weeks ago

The problem is that this behaviour is inconsistent. some actions provide suggestions to a CLI user, and some do not.

This is not true.

The thing here is that the "action" in the substitution on the cli does provide the suggestion. For example it would give output like the below.

The behavior you're describing here is not action-related. It's how the substitution extension point works.

The following rule (no action defined) will give the same results.

extends: substitution
message: "Use '%s' instead of '%s'."
level: error
swap:
  chicken: chimken
Firstyear commented 2 weeks ago

I think the documentation around actions needs to much more clearly state that they do not influence the CLI output or suggestions, and they are only for language server actions and external tools.

Otherwise, we think that this severely limits what we can do with vale, since we can not enforce that everyone uses a language server, meaning that there is effectively no reason to create or invest in actions.

If you want, you can consider this a feature request that actions are executed and act as suggestions in the main vale cli, rather than being limited as something for a language server.

ccoVeille commented 2 weeks ago

I totally agree and support @Firstyear

I thought the code was unable to handle it, but apparently the LSP server does, so the information is already available. So I assume (No computer with me right now, I cannot check) it would be visible when using --output JSON when using CLI.

Is there any limitations that would make it impossible? If we exclude it has to be coded, and maintained.

I mean the regexp there allows to use $1, $2… something substitution cannot do. So the regexp feature here is great. And the LSP already allow fixing it.

Why the CLI wouldn't be able to display it?

I would expect it to be possible.

Firstyear commented 2 weeks ago

I thought the code was unable to handle it, but apparently the LSP server does, so the information is already available. So I assume (No computer with me right now, I cannot check) it would be visible when using --output JSON when using CLI.

the --output json just emits the regex and the location for the external tool to apply the regex over the text. It doesn't actually perform the regex itself.

ccoVeille commented 2 weeks ago

Thanks for checking @Firstyear. Let's wait @jdkato reply.

I would like to get a feedback like this:

Because right now, it's unclear to me

jdkato commented 1 week ago

I understand why this would be useful, but it's not a simple change. I'll look into it when I get around to addressing https://github.com/errata-ai/vale/issues/612.

Firstyear commented 1 week ago

Thank you!