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

Update: remove suggestion if it didn't provide a fix (fixes #13723) #13772

Merged
merged 1 commit into from Oct 23, 2020

Conversation

mdjermanovic
Copy link
Member

Prerequisites checklist

What is the purpose of this pull request? (put an "X" next to an item)

[ ] Documentation update
[X] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

fixes #13723

Marked this change as an "Update", because it can cause some existing tests in plugins to fail.

What changes did you make? (Give an overview)

Modified ReportTranslator to automatically remove the whole suggestion instance from the linting output if the suggestion's fix function returned null or an empty array/sequence.

Context:

This change prevents invalid lint messages.

ESLint API defines Suggestion#fix as a mandatory field in a LintMessage. This means that all suggestions produced by the linting must include fixes. Integrations, following the spec, expect that this property exists and has a valid EditInfo value. For example, VSCode crashes if it is null.

ReportTranslator already throws if the fix property in a suggestion object provided by the rule isn't a function, but it wasn't checking the value returned from that function.

It's still allowed to return null or return [] from fix(), or yield nothing from *fix(), as an indicator that suggestion's fix cannot be applied. This is a commonly used pattern in auto-fix fix functions (e.g., when it finds out that some comments in the code are interfering with the fix).

After this change, instead of creating an invalid suggestion object with fix: null, the whole suggestion will be removed from the final output. If there are multiple suggestions for the same error, only those that didn't provide a fix will be removed.

Is there anything you'd like reviewers to focus on?

@mdjermanovic mdjermanovic added bug ESLint is working incorrectly enhancement This change enhances an existing feature of ESLint core Relates to ESLint's core APIs and features accepted There is consensus among the team that this change meets the criteria for inclusion labels Oct 19, 2020
Copy link
Member

@btmills btmills left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @mdjermanovic! When I saw +233/-9 in the diff, I was pleasantly surprised to see only a single new .filter() in the implementation with the rest being all tests 😆

@btmills btmills merged commit bfddced into master Oct 23, 2020
@btmills btmills deleted the issue13723 branch October 23, 2020 23:40
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Apr 22, 2021
@eslint-github-bot eslint-github-bot bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Apr 22, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion bug ESLint is working incorrectly core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RuleTester: Cannot test failing suggestion fixer
2 participants