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

@mdjermanovic mdjermanovic commented Oct 19, 2020

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?

Copy link
Member

@btmills btmills left a comment

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
12 checks passed
12 checks passed
Verify Files
Details
Test (ubuntu-latest, 14.x)
Details
Test (ubuntu-latest, 13.x)
Details
Test (ubuntu-latest, 12.x)
Details
Test (ubuntu-latest, 10.x)
Details
Test (ubuntu-latest, 10.12.0)
Details
Test (windows-latest, 12.x)
Details
Test (macOS-latest, 12.x)
Details
Browser Test
Details
commit-message Commit message follows guidelines
Details
licence/cla Contributor License Agreement is signed.
Details
release-monitor No patch release is pending
Details
@btmills btmills deleted the issue13723 branch Oct 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants
You can’t perform that action at this time.