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: Add disallowTemplateShorthand option in no-implicit-coercion #13579

Merged

Conversation

@remcohaszing
Copy link
Contributor

@remcohaszing remcohaszing commented Aug 17, 2020

Prerequisites checklist

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

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

What changes did you make? (Give an overview)

This adds the templateString option to no-implicit-coercion. This makes the
rule report the following code:

`${foo}`

For backwards compatibility, this was added as a separate option instead of as
default behaviour.

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

Closes #12866

@kaicataldo
Copy link
Member

@kaicataldo kaicataldo commented Aug 18, 2020

Since the linked issue wasn't accepted, we'll need to go through the acceptance process in this PR. I've added my support for the proposal.

Loading

@remcohaszing
Copy link
Contributor Author

@remcohaszing remcohaszing commented Aug 29, 2020

This also resolves a race condition in the fixers of prefer-template and no-implicit-coercion.

Input

'' + foo;

Output when autofixed by prefer-template:

`${foo}`;

Output by no-implicit-coercion

String(foo);

This option will still trigger no-implicit-coercion after prefer-template has run its autofixer, meaning there is only one correct way to write this string conversion if both rules are enabled.

Loading

@remcohaszing
Copy link
Contributor Author

@remcohaszing remcohaszing commented Nov 2, 2020

I’m not getting a lot of feedback. As far as I can tell this pull request can be merged. Does this require any actions from my side?

Loading

@mdjermanovic
Copy link
Member

@mdjermanovic mdjermanovic commented Nov 2, 2020

@remcohaszing this enhancement hasn't been accepted yet, per our process it still needs a champion from the team.

Loading

docs/rules/no-implicit-coercion.md Outdated Show resolved Hide resolved
Loading
@mdjermanovic
Copy link
Member

@mdjermanovic mdjermanovic commented Mar 26, 2021

I can champion this.

Loading

docs/rules/no-implicit-coercion.md Outdated Show resolved Hide resolved
Loading
docs/rules/no-implicit-coercion.md Outdated Show resolved Hide resolved
Loading
docs/rules/no-implicit-coercion.md Outdated Show resolved Hide resolved
Loading
docs/rules/no-implicit-coercion.md Show resolved Hide resolved
Loading
docs/rules/no-implicit-coercion.md Outdated Show resolved Hide resolved
Loading
docs/rules/no-implicit-coercion.md Outdated Show resolved Hide resolved
Loading
lib/rules/no-implicit-coercion.js Show resolved Hide resolved
Loading
tests/lib/rules/no-implicit-coercion.js Show resolved Hide resolved
Loading
lib/rules/no-implicit-coercion.js Outdated Show resolved Hide resolved
Loading
lib/rules/no-implicit-coercion.js Outdated Show resolved Hide resolved
Loading
remcohaszing added 2 commits Apr 5, 2021
…nt#12866)

This adds the `templateString` option to `no-implicit-coercion`. This makes the
rule report the following code:

```js
`${foo}`
```

For backwards compatibility, this was added as a separate option instead of as
default behaviour.
@remcohaszing remcohaszing force-pushed the no-implicit-coercion-template-string branch from 6f9144d to bdfe13e Apr 5, 2021
docs/rules/no-implicit-coercion.md Outdated Show resolved Hide resolved
Loading
@mdjermanovic mdjermanovic changed the title Update: Add templateString option in no-implicit-coercion (fixes #12866) Update: Add disallowTemplateShorthand option in no-implicit-coercion Apr 6, 2021
Copy link
Member

@mdjermanovic mdjermanovic left a comment

@remcohaszing thanks for the quick responses! This looks good, I have only some small suggestions about the documentation.

Loading

docs/rules/no-implicit-coercion.md Outdated Show resolved Hide resolved
Loading
docs/rules/no-implicit-coercion.md Show resolved Hide resolved
Loading
remcohaszing and others added 2 commits Apr 6, 2021
Copy link
Member

@mdjermanovic mdjermanovic left a comment

LGTM, thanks!

Loading

@mdjermanovic mdjermanovic merged commit f06ecdf into eslint:master Apr 9, 2021
13 checks passed
Loading
@mdjermanovic
Copy link
Member

@mdjermanovic mdjermanovic commented Apr 9, 2021

Thanks for contributing!

Loading

@remcohaszing
Copy link
Contributor Author

@remcohaszing remcohaszing commented Apr 9, 2021

Thanks for your code reviews!

Loading

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants