Fix: no-implicit-coercion string concat false positive (fixes #7057)

[kaicataldo]

The rule was checking that the Literal is not a number in the case of +foo (so it would not warn for +42), but was not doing the same for strings.

[mention-bot]

@kaicataldo, thanks for your PR! By analyzing the annotation information on this pull request, we identified @mysticatea, @ipanasenko and @vitorbal to be potential reviewers

[eslintbot]

LGTM

[TheSavior]

Thanks for doing this!

[platinumazure]

Consider using isStringLiteral from ast-utils? (One significant difference, that function also checks for TemplateLiterals. I think that might be okay here, though.)

[kaicataldo]

Oh cool! Didn't know about this - thanks!

[platinumazure]

Left a suggestion about using an ast-utils method and adding a test.

In addition: If you do follow my suggestion and use ast-utils' isStringLiteral method, since that also allows TemplateLiteral nodes, could you add some representative tests that use template strings in place of strings in existing valid/invalid tests?

Thanks, otherwise looks great!

[kaicataldo]

Updated with suggested changes - thanks!

[eslintbot]

LGTM

[platinumazure]

Thanks @kaicataldo!

I was originally thinking of tests like this:

foo + ``;
`` + foo;

Maybe I'm missing something-- are empty TemplateLiterals syntactically invalid?

[kaicataldo]

This is something that the rule does not currently check - do we want to lump that change into this bug fix?

[kaicataldo]

Updated - this has become a sort of enhancement/secondary bug fix in addition to the bug I set out to fix, in that it is now updated to check ES6 TemplateLiterals. I'm fine with doing both in the same PR, but would like to make sure others are okay with it too since it wasn't originally discussed in the issue.

[eslintbot]

LGTM

[platinumazure]

Sorry, I didn't realize templates weren't flagged by this!

On Sep 5, 2016 6:03 PM, "ESLint Bot" notifications@github.com wrote:

LGTM

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#7060 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AARWej94oj8U_lvyaw6-PX7rYGlbi-pKks5qnJ_EgaJpZM4J1PEI
.

[kaicataldo]

Seems like a worthwhile update (I think this rule was written before ES6), just want to make sure it doesn't accidentally get snuck in without consideration. Merging this change would constitute a minor bump instead of a patch.

Happy to revert the suggested changes and split this up into two PRs with a separate issue for adding TemplateLiteral support if we decide we want to keep these two separate (will update the issue/PR names if we decide to go this route). Thought I'd try discussing on here, since we talked about not requiring issues for every change anymore!

@eslint/eslint-team Thoughts?

[eslintbot]

LGTM

[TheSavior]

It seems like the easier and less controversial approach would be to split this into two PRs.

[platinumazure]

@kaicataldo Sorry, this came up because I wanted you to use ast-utils. 😄 If you want to revert that change so this PR remains a bugfix, please go ahead. I'll write an enhancement request issue for enforcing on template literals as well.

[kaicataldo]

No need to apologize :)

Alright, let's split this up. Will revert to the original PR and we can discuss adding support for template strings in a separate issue.

[kaicataldo]

Reverted to just the original bug fix (but still using astUtils.isStringLiteral() - I still would prefer to use the built-in utility function with an extra guard than re-implement). I checked out a new branch with the TemplateLiteral check and tests, so if we decide to implement that it'll be quite simple!

[eslintbot]

LGTM

[kaicataldo]

@platinumazure How does this look now? Want to make sure I get your feedback since you commented on the original iteration!

[platinumazure]

LGTM, thanks!

[vitorbal]

LGTM

[ilyavolodin]

LGTM. Thanks