Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add fixes property #13

Open
4 tasks done
remcohaszing opened this issue Dec 18, 2021 · 6 comments
Open
4 tasks done

Add fixes property #13

remcohaszing opened this issue Dec 18, 2021 · 6 comments
Labels
🤞 phase/open Post is being triaged manually

Comments

@remcohaszing
Copy link
Member

remcohaszing commented Dec 18, 2021

Initial checklist

Problem

I have been working on unifiedjs/unified-language-server#13. On top of those changes I have been developing quick fixes based on the expected property of vfile messages. This works, but it’s very limited.

Screencast_2021-12-18_12.38.29.mp4

It now works by replacing the entire reported unist position with a string value from the expected array.

This works fine for retext-sentence-spacing:double-space, because it reports a small range. Although a more descriptive text for the action would be nice. All properties on vfile messages can be used to construct this description.

Currently this is: 'Fix: ' + diagnostic.message

Probably better alternatives would be something like: 'Replace “' + actual + '” with + “' + expected + '”'

However, for remark-lint:unordered-list-marker-style this is going to be harder to do, because the entire node is reported. It would be nice to only use the first character for autofixing.

I’m missing the following data for making a proper quick fix:

  1. A human readable description of the quick fix.
  2. A custom range for the quick fix.

Solution

Add a new property fixes which should contain an array of the following shape:

interface VFileMessageFix {
  description?: string;
  position: import('unist').Position;
  replacement: string;
}

Alternatives

It would be possible to support custom fixer functions instead of or in addition of text replacements.


Report smaller ranges.

remark-lint:unordered-list-marker-style is a good example of a rule which could narrow the reported range to a single character (the actual list marker).

Even then a custom description for the change could be nice.


Reuse the expected field to allow this shape in addition to strings.

@github-actions github-actions bot added 👋 phase/new Post is being triaged automatically 🤞 phase/open Post is being triaged manually and removed 👋 phase/new Post is being triaged automatically labels Dec 18, 2021
@ChristianMurphy
Copy link
Member

Crosslinking, this is likely related to remarkjs/remark-lint#82

@wooorm
Copy link
Member

wooorm commented Dec 19, 2021

why isn’t a message’s description, the message’s position, and one of the expected enough?

@remcohaszing
Copy link
Member Author

The message description shouldn’t be used for the fix. I used this in the OP, but it doesn’t work, because there may be multiple fixes available, meaning the user has to choose between: Fix: A and Fix: A.

So let’s say the default message will be: 'Replace “' + actual + '” with + “' + expected + '”'.

This message can be fine in some causes. I.e.:

Kirby sucks!
      ^^^^^-- message: That’s not nice!
               |-> Replace “sucks” with “rocks”
               |-> Replace “sucks” with “vacuums”

However, in some cases this can be annoying. I.e. let’s say we have a remark rule to sort link definitions alphabetically and the following content:

[unifiedjs]: https://unifiedjs.com

[remark]: https://remark.js.org

Ideally only the link reference name (or possibly the entire link reference) is reported:

[unifiedjs]: https://unifiedjs.com

[remark]: https://remark.js.org
 ^^^^^^-- message: “remark” link reference should be sorted before “unifiedjs”

However, to get an autofix working, the text range needs to span both link references.

[unifiedjs]: https://unifiedjs.com
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

^
[remark]: https://remark.js.org
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^-- message: “remark” link reference should be sorted before “unifiedjs”

The autogenerated autofix description would look like this:

Replace “[unifiedjs]: https://unifiedjs.com\n\n[remark]: https://remark.js.org” with “[remark]: https://remark.js.org\n\n[unifiedjs]: https://unifiedjs.com”

A custom fixer range could make the error less verbose. A custom fixer message could be something like: Sort “remark” before “unifiedjs”, which is much easier to read.

@remcohaszing
Copy link
Member Author

For comparison: ESLint supports fixes and suggestions. Each report message may contain 1 fix and multiple suggestions. The fix has no description, but it does specify its own range. Suggestions define both a user readable message and a fixer, which also specifies a specific range.

Since there may be multiple vfile messages expected is an array, I think this is more similar to ESLint suggestions than autofixers.

@wooorm
Copy link
Member

wooorm commented Dec 27, 2021

Perhaps it’s good to look at practical examples of how this would be used? There are several problems with string-based suggestions, that the ESLint folks also see, and I mentioned in the thread linked by Christian before. These problems become much bigger for us compared to ESLint because we do two things (a. messages, b. AST based transforms/format)

@wooorm
Copy link
Member

wooorm commented Oct 27, 2022

still open to ideas, currently it’s not yet clear to me how it’ll work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🤞 phase/open Post is being triaged manually
Development

No branches or pull requests

3 participants