Option to Fix References

samjcombs commented 1 year ago

Initial checklist

[X] I read the support docs
[X] I read the contributing guide
[X] I agree to follow the code of conduct
[X] I searched issues and couldn’t find anything (or linked relevant results below)

Problem

It would be great if these three rules:

no-undefined-references
no-duplicate-definitions
no-unused-definitions had a --fix or --output option to do the following:
no-undefined-references - remove the brackets around the undefined reference
no-duplicate-definitions - add a comment to each duplicate so the user can decide which one to keep
no-unused-definitions - remove the definition entirely

I have been looking into how to achieve this but before I dive in completely I was wondering if theres a simple option to turn a rule into a write rule instead of just a warning reporter.

Great work on this repo, very impressive stuff here!

Solution

For `no-undefined-references`

Given this input:

Welcome to [foo].

Desired output:

Welcome to foo.

For `no-duplicate-definitions`

Given this input:

Welcome to [foo].
[foo]: http://foo.com/1
[foo]: http://foo.com/2

Desired output:

Welcome to [foo].
[foo]: http://foo.com/1 // Duplicate definition `foo` defined on line 3. 
[foo]: http://foo.com/2 // Duplicate definition `foo` defined on line 2.

For `no-unused-definitions`

Given this input:

Welcome to foo.
[bar]: http://foo.com/1

Desired output:

Welcome to foo.

Alternatives

I tried piping the output of the cli with these three rules enabled to a script that would parse it and determine where to do file edits, but it got too complicated as lots of work has to be done to make sure that when it starts editing the file, future edits take into account what was already removed and translate positions of everything accordingly. All in all, not a good solution.

Another option I've been toying with is to use these rules to output the AST tree instead of the files, and then parse through that AST and perform the modifications, but I'm not sure how I'd find the violating AST nodes, unless these rules actually add isAllowed to the ast nodes themselves.

Open to other solutions here and it may be easier to do this on a rule by rule basis.

But an abstract solution to provide a fix and fix options for each rule would be great, so users could say, for nodes that violate this rule, fix it in this way.

Thanks much!

ChristianMurphy commented 1 year ago

Welcome @samjcombs! :wave: Thanks for the time and consideration, improving the developer experience is definitely a continuing goal for the project.

I have been looking into how to achieve this but before I dive in completely I was wondering if theres a simple option to turn a rule into a write rule instead of just a warning reporter.

Not currently, see https://github.com/remarkjs/remark-lint/issues/82 There is ongoing discussion on the approach, your thoughts and insights are welcome on the thread.

no-undefined-references no-unused-definitions

I'd be cautious outright removing the reference or definitions as a fix. I see there being (at least) three cases.

There is plain text with a parenthetical which happens to use square brackets, the appropriate fix here would be to escape to clarify that it cannot be a reference ([aside] becomes \[aside\])
There a reference is intended but there is a typo ([a-real-reference-typo] should suggest, "reference is undefined did you mean [a-real-reference]?")
Definition was intentionally removed, but a reference is still hanging (the situation you note)

Given that there are multiple scenarios, I'm not sure remark-lint should take automatic action even if it could. This feels like something where an integration with https://github.com/remarkjs/vscode-remark would be a better fit, allowing the user to choose from a list of potential fixes.

no-duplicate-definitions

I'm not sure the value of adding a comment would have here. It doesn't fix the error, and it contains the same information an error message would have. So the error message may be fine?

Given your interest in fixes. I'd highly recommend reading through https://github.com/remarkjs/remark-lint/issues/82 and chiming in with ideas. As well as checking out https://github.com/remarkjs/vscode-remark, I could definitely see a tie-in between multiple fix options, which require user feedback, and the language server.

ChristianMurphy commented 1 year ago

@samjcombs thoughts on the above? Is thing something you'd be interested in working on?

remcohaszing commented 1 year ago

remark-language-server / vscode-remark gives suggestions based on the expected property of reported messages. Let’s say you have this message, it will show a quick fix suggestion to remove the reference:

const message = new VFileMessage('Undefined reference [foo]', nodeOrPosition, 'remark-lint:no-undefined-references')
message.expected = ['foo']

For removal, suggest an empty string.

const message = new VFileMessage('Duplicate reference [foo]', nodeOrPosition, 'remark-lint:no-duplicate-definitions')
message.expected = ['']

This expected property is provided by quite a lot of retext plugins, but not remark-lint plugins. I definitely see value to add several of those.

This doesn’t provide an option to fix all issues with a single command though.

wooorm commented 10 months ago

Closing as is discussed in GH-82.

Adding actual/expected fields can be done in separately PRs!

wooorm commented 10 months ago

Duplicate of #82

github-actions[bot] commented 10 months ago

Hi! This was closed. Team: If this was fixed, please add phase/solved. Otherwise, please add one of the no/* labels.

github-actions[bot] commented 10 months ago

Hi! Thanks for taking the time to contribute!

Because we treat issues as our backlog, we close duplicates to focus our work and not have to touch the same chunk of code for the same reason multiple times. This is also why we may mark something as duplicate that isn’t an exact duplicate but is closely related.

Thanks, — bb

remarkjs / remark-lint