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.

Sign up for GitHub

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

Jump to bottom

fix: improve fixer of prefer-template rule #15230

Merged
merged 1 commit into from
Dec 17, 2021
Merged

fix: improve fixer of prefer-template rule #15230

merged 1 commit into from
Dec 17, 2021

Conversation

snitin315
Copy link
Contributor

@snitin315 snitin315 commented Oct 29, 2021 •
edited
Loading

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 auto fixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

What changes did you make? (Give an overview)

Resolve #15083

Before

code: var foo = "Hello " + "world " + test => fixed code: var foo = ${"Hello " + "world "}${test}

After

code: var foo = "Hello " + "world " + test => fixed code: var foo = Hello world ${ test}

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

No

@eslint-github-bot eslint-github-bot bot added the triage An ESLint team member will look at this issue soon label Oct 29, 2021
@snitin315 snitin315 changed the title fix: improve fixer of prefer-template fix: improve fixer of prefer-template rule Oct 29, 2021
@snitin315 snitin315 marked this pull request as ready for review October 29, 2021 01:02
mdjermanovic
mdjermanovic requested changes Oct 29, 2021
View reviewed changes
lib/rules/prefer-template.js Outdated Show resolved Hide resolved
@mdjermanovic mdjermanovic added accepted There is consensus among the team that this change meets the criteria for inclusion autofix This change is related to ESLint's autofixing capabilities bug ESLint is working incorrectly rule Relates to ESLint's core rules and removed triage An ESLint team member will look at this issue soon labels Oct 29, 2021
@snitin315 snitin315 force-pushed the fix/improve-prefer-template-autofixer branch from e5fdb7c to 35ee14a Compare October 29, 2021 11:21
mdjermanovic
mdjermanovic requested changes Oct 30, 2021
View reviewed changes
lib/rules/prefer-template.js Outdated Show resolved Hide resolved
lib/rules/prefer-template.js Outdated Show resolved Hide resolved
mdjermanovic
mdjermanovic requested changes Oct 30, 2021
View reviewed changes
lib/rules/prefer-template.js Outdated Show resolved Hide resolved
mdjermanovic
mdjermanovic requested changes Nov 5, 2021
View reviewed changes
lib/rules/prefer-template.js Outdated Show resolved Hide resolved
lib/rules/prefer-template.js Outdated Show resolved Hide resolved
@snitin315 snitin315 marked this pull request as draft November 7, 2021 03:49
@snitin315 snitin315 force-pushed the fix/improve-prefer-template-autofixer branch from 55df3b3 to 00d053c Compare November 14, 2021 03:04
@snitin315 snitin315 marked this pull request as ready for review November 14, 2021 03:10
mdjermanovic
mdjermanovic requested changes Dec 3, 2021
View reviewed changes
lib/rules/prefer-template.js Outdated Show resolved Hide resolved
lib/rules/prefer-template.js Outdated Show resolved Hide resolved
lib/rules/prefer-template.js Outdated Show resolved Hide resolved
@snitin315 snitin315 force-pushed the fix/improve-prefer-template-autofixer branch from 7b0e9d9 to db2c7b6 Compare December 5, 2021 11:41
mdjermanovic
mdjermanovic approved these changes Dec 16, 2021
View reviewed changes
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.

LGTM, thanks!

I think this is a good improvement over the current behavior. A further improvement we could think about in the future could be to not change string literals that don't have to be converted to template literals.

I'll leave this open for a couple more days in case anyone else wants to review it.

@mdjermanovic mdjermanovic merged commit 7d832d4 into main Dec 17, 2021
@mdjermanovic mdjermanovic deleted the fix/improve-prefer-template-autofixer branch December 17, 2021 22:12
@rokerkony
Copy link

@snitin315 @mdjermanovic thank you guys for the fix, do you plan to fix also the additional empty space after opening bracket${ ?

so instead of

var foo = Hello world ${ test}

it would generate

var foo = Hello world ${test}

Shell I create a new issue for it?

@mdjermanovic
Copy link
Member

do you plan to fix also the additional empty space after opening bracket${ ?

so instead of

var foo = Hello world ${ test}

it would generate

var foo = Hello world ${test}

No, that works as intended for this rule because it doesn't know preferences about spacing in ${}, and that spacing can be auto-fixed by other rules in subsequent iterations. If you enable template-curly-spacing with "never" (default), "Hello world " + test will be eventually autofixed to `Hello world ${test}`, here's an online demo example.

@mdjermanovic
Copy link
Member

here's an online demo example.

(see the Fixed Code tab)

@rokerkony
Copy link

@mdjermanovic thank you for the crazy fast response and one more rule to our collection :) thank you

@sync-by-unito sync-by-unito bot mentioned this pull request Jan 1, 2022
Closed
@sync-by-unito sync-by-unito bot mentioned this pull request Feb 1, 2022
Closed
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Jun 16, 2022
@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 Jun 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mdjermanovic mdjermanovic mdjermanovic approved these changes

Assignees
No one assigned
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 autofix This change is related to ESLint's autofixing capabilities bug ESLint is working incorrectly rule Relates to ESLint's core rules
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Bug: Probably wrong fix of ESLint's prefer-template warning
3 participants
@snitin315 @rokerkony @mdjermanovic