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

feat: Notify Plugins on Fix #93

Closed
wants to merge 1 commit into from
Closed

Conversation

Jason3S
Copy link

@Jason3S Jason3S commented Sep 19, 2022

Summary

Notify Plugins when a fix has been applied. Currently a plugin has no way to know if a fix has been applied.

Related Issues

eslint/eslint#16143

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Sep 19, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: Jason3S / name: Jason Dent (817039f)

@eslint-github-bot
Copy link

Hi @Jason3S!, thanks for the Pull Request

The first commit message isn't properly formatted. We ask that you update the message to match this format, as we use it to generate changelogs and automate releases.

  • The commit message tag wasn't recognized. Did you mean "docs", "fix", or "feat"?
  • There should be a space following the initial tag and colon, for example 'feat: Message'.
  • The first letter of the tag should be in lowercase

To Fix: You can fix this problem by running git commit --amend, editing your commit message, and then running git push -f to update this pull request.

Read more about contributing to ESLint here

@Jason3S Jason3S changed the title RFC: Notify Plugins on Fix feat: Notify Plugins on Fix Sep 19, 2022
@eslint-github-bot
Copy link

Hi @Jason3S!, thanks for the Pull Request

The first commit message isn't properly formatted. We ask that you update the message to match this format, as we use it to generate changelogs and automate releases.

  • The commit message tag wasn't recognized. Did you mean "docs", "fix", or "feat"?
  • There should be a space following the initial tag and colon, for example 'feat: Message'.
  • The first letter of the tag should be in lowercase

To Fix: You can fix this problem by running git commit --amend, editing your commit message, and then running git push -f to update this pull request.

Read more about contributing to ESLint here

@Jason3S Jason3S changed the title feat: Notify Plugins on Fix feat(eslint): Notify Plugins on Fix Sep 19, 2022
@eslint-github-bot
Copy link

Hi @Jason3S!, thanks for the Pull Request

The first commit message isn't properly formatted. We ask that you update the message to match this format, as we use it to generate changelogs and automate releases.

  • The commit message tag wasn't recognized. Did you mean "docs", "fix", or "feat"?
  • There should be a space following the initial tag and colon, for example 'feat: Message'.
  • The first letter of the tag should be in lowercase

To Fix: You can fix this problem by running git commit --amend, editing your commit message, and then running git push -f to update this pull request.

Read more about contributing to ESLint here

@mdjermanovic mdjermanovic changed the title feat(eslint): Notify Plugins on Fix feat: Notify Plugins on Fix Sep 19, 2022
@mdjermanovic mdjermanovic added Initial Commenting This RFC is in the initial feedback stage and removed triage labels Sep 19, 2022
Copy link
Member

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the RFC!

It's not clear to me whether this feature relates to autofixes or suggestions or both, so we should clarify that in the RFC. The provided example (adding the word to the custom dictionary) would probably be a suggestion rather than autofix.

Comment on lines +20 to +24
When used with an editor extension like VS Code ESLint, something like the following would show up:

<img width="402" alt="image" src="./quick-fix.png">

If the fix is applied, the spellchecker plugin would add the word to the custom dictionary.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VS Code applies individual autofixes and suggestions independently of ESLint, based on the provided EditInfo. ESLint doesn't know if the fix/suggestion is applied in the editor. It seems that we should provide an API for editors to notify ESLint when the fix/suggestion has been applied.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not clear to me whether this feature relates to autofixes or suggestions or both.

In the spell checker case, it is explicitly for suggestions over autofixes.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VS Code applies individual autofixes and suggestions independently of ESLint, based on the provided EditInfo. ESLint doesn't know if the fix/suggestion is applied in the editor. It seems that we should provide an API for editors to notify ESLint when the fix/suggestion has been applied.

Thank you for pointing this out. I hadn't realized that they didn't use an API to make the change.

The applied method could act as an API to ESLint to make it known that a suggestion was used.

This is useful generating stats / metrics on which suggestions are used more frequently.

But, this changes the "scope" of this RFC beyond just ESLint to tell a plugin if a suggestion was used.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dbaeumer,

Do you have a suggestion on an API for letting ESLint + plugins known that a suggestion was used?

Copy link

@bradzacher bradzacher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The big problem I see with this proposal is that it runs in the assumption that (a) eslint is the sole controller of all fixes and (b) eslint fixes are created and applied in a single thread.

(a) does not hold true for IDE integrations because there is no API provided by ESLint to apply just one fix result to a code string. There is ESLint.outputFixes, but that writes to disk automatically, which isn't desirable for an IDE action. This means IDEs have to implement their own solution for applying a single fix to a string.
VSCode-eslint also does things like "autofix all fixes for this particular rule in the file", which is another thing that's not possible with ESLint APIs.

I mention (b) because functions are not JSON serializable and the main way of communicating between JS threads/processes (like the LSP protocol) is via JSON. I'm not sure about vscode-eslint, but I have seen IDE implentations that run eslint in its own thread and apply fixes in a separate thread.

@Jason3S
Copy link
Author

Jason3S commented Sep 19, 2022

The big problem I see with this proposal is that it runs in the assumption that (a) eslint is the sole controller of all fixes and (b) eslint fixes are created and applied in a single thread.

Excellent points and I agree that both of them are an issue. (b) effectively makes passing a function a non-starter.

As it happens, the VSCode plugin runs everything in the LSP and only sends the selected edit to the editor. But depending upon a certain behavior is not a good idea.

From the feedback, this RFC proposal in its current form is dead in the water.

Here is the situation:

ESLint has become the de facto "linter" API for JavaScript, TypeScript, and a host of other file types. As a result, it is getting asked to do things like "spell check a file" or make the code look pretty. ESLint is effectively the API glue between a host of linters and editor extensions that use the Suggestions to make fixes. This enabling effect has grown because ESLint has provided a standard has been easy enough to use and leverage beyond its original scope.

Which brings us to the reason for this RFC. What had seemed like a simple enough feature request: ESLint Plugin: Configuration option to add words to the dictionary · Issue #3233 · streetsidesoftware/cspell turned out to not be possible.

Maybe it is beyond the scope of ESLint, but it could be leveraged as the interface layer between linter plugins and editor extensions to apply edit suggestions that go beyond simple changes to a file by providing a way to send messages to the plugins.

@mdjermanovic
Copy link
Member

It seems we need information from the vscode-eslint team and/or other integrations on whether it is possible to call a function after the suggested fix is applied. If that is not possible, then this proposal in its current form doesn't seem feasible.

Another thing to consider is that while the initial idea is to add this to Suggestions, the original use case is not actually a code fix. If ESLint would provide the callback function as a different entity unrelated to code fixes, e.g. in a new property actions: Array<{ description, callback }> on LintMessage objects, maybe that is something that integrations could support.

@dbaeumer
Copy link

dbaeumer commented Oct 4, 2022

Actually letting ESLint know that a fix / suggestion was used will be hard. VS Code doesn't provide any callback when a code action is applied. The VS Code ESLint extension basically converts fixes into VS Code's text edits and hands them over to VS Code. The user then selects one from a list and VS Code applies it. All that the extension currently gets is a change event on the document as a result.

@nzakas
Copy link
Member

nzakas commented Oct 12, 2022

It seems like there is a consensus that this RFC can’t move forward, so closing. If anyone wants to continue, it would be best to do so in an issue or discussion.

@nzakas nzakas closed this Oct 12, 2022
@Jason3S Jason3S deleted the notify-plugins branch October 13, 2022 05:53
@Jason3S
Copy link
Author

Jason3S commented Oct 13, 2022

Thank you for the consideration. I agree that this RFC cannot go forward.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Initial Commenting This RFC is in the initial feedback stage
Projects
None yet
5 participants