New: no-template-curly-in-string rule (fixes #6186) #6767

jfmengels · 2016-07-26T19:03:29Z

Adds the new rule wrong-quotes-for-template-string, and fixes #6186.

Let me know if and what I need to change or if I missed a step.

Notes:

I turned the rule off in conf/eslint.json, as it triggered a lot of errors
I put the rule into the Possible Errors category, which sounds like the most appropriate to me
I made it recommended, but maybe you only add new recommended rules on major versions. Should I turn it off?

mention-bot · 2016-07-26T19:03:30Z

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

eslintbot · 2016-07-26T19:03:30Z

LGTM

platinumazure · 2016-07-27T01:14:00Z

We definitely only change eslint:recommended on major releases.

Also, could you provide a link to an overview of the prefixed template string syntax and associated semantics? Just for my own education if nothing else 😄

mysticatea · 2016-07-27T02:03:03Z

lib/rules/wrong-quotes-for-template-string.js

+        return {
+            Literal: function(node) {
+                if (typeof node.value === "string" && regex.test(node.value)) {
+                    context.report(node, "Possible use of template string inside a regular string.");


This overload is old style.
The following code is more proper 😃

context.report({ node: node, message: "Possible use of template string inside a regular string." });

http://eslint.org/docs/developer-guide/working-with-rules#contextreport

Ha, that's what I usually use, but I checked an existing rule to be consistent with the project style, and must have copied it from one that wasn't updated 😅

Am I allowed to use object literal syntax?

context.report({ node, message: "Possible use of template string inside a regular string." });

Sidenote: for my own plugins, I found it pretty nice to have the following syntax, but didn't use it as it isn't the norm here.

const create = (context) => {} // of function create() {}, whatever const schema = []; module.exports = { create, meta: { schema, docs: { description: "Warn when using template string syntax in regular strings", category: "Possible Errors", recommended: true } } };

Don't plan on adding it here now, but food for thought.

I think we can use property shorthands (except get and set. {get, set} will cause syntax error for Node 4).
But it might be modified when we enable object-shorthand rule (related in #6407).

(FWIW, I would not like arrow functions here.)

mysticatea · 2016-07-27T02:25:03Z

Looks good to me (except a nit pick), thank you!

(Though I'm happier if this rule is fixable 😜)

lukeapage · 2016-07-27T03:13:44Z

Fyi: lodash supports es2015 syntax in its template, so thats a reason to maybe not make it recomended?

jfmengels · 2016-07-27T09:23:08Z

So am I understanding it right that I should set recommended to false?

Also, since ESLint now doesn't support 0.1X versions of Node.js, can and should I use template literals myself, const, etc. where appropriate?

@platinumazure See https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Template_literals#Tagged_template_literals. Should I put it in the rule description? It's not all that relevant to this rule IMO, as you can't put wrong quotes with it templateFunction'${foo}' as that would be a syntax error.

@lukeapage Huh, didn't even know that. From what I understand, I should not set it as recommended anyway, but it's a valid concern. Should I add this use case in the When not to use it section, or a note somewhere else? Sounds like a pretty limited scenario that would fit a eslint-disable comment IMO, but might be worth noting.

@mysticatea About the fixing part, I don't mind doing it, but I have a few concerns.
From what I understand, I should take the string literal and replace it by the same string but replace it by something that has backtick quotes.
Should I replace any \' / \" that are inside the string (or at least the ones corresponding to the used quotes).
Should I prepend any backtick quotes by a \?
For the last two questions, should I do that in the literal parts and/or in the place holders? I'd say both, but I'll have to test it out well before being sure.

mysticatea · 2016-07-27T10:07:45Z

So am I understanding it right that I should set recommended to false?

Yes. we cannot change eslint:recommended for now.
When we do a major update, like #6403 and #4953, we will evaluate rules and will change eslint:recommended.

About the fixing part, I don't mind doing it, but I have a few concerns.

Yes, the fixing is difficult.
We can do it carefully after this PR is merged.

eslintbot · 2016-07-27T11:49:34Z

LGTM

jfmengels · 2016-07-27T11:49:56Z

Updated with regards to @mysticatea's comments.

Yes, the fixing is difficult. We can do it carefully after this PR is merged.

Let's do that. I can help with that again :)

nzakas · 2016-07-27T19:13:08Z

lib/rules/wrong-quotes-for-template-string.js

+    },
+
+    create: function(context) {
+        var regex = /\$\{.*\}/;


Maybe better to do [^/}]+ instead of .* (more specific)

Do you mean [^}]? I agree, that would be better 👍

nzakas · 2016-07-27T19:17:06Z

Code looks okay, but I think we need to rethink the rule name and description. This rule isn't about quotes, it's about embedded expressions, and I think it should be warning about that. So maybe no-confusing-substring or something similar? And the warning would be something like, "Unexpected template string expression."

jfmengels · 2016-07-27T21:08:53Z

For the rule name, IMO ideally it would be no-template-literal-placeholder-in-string, ...but it's too long. I would be fine with no-confusing-substring, but I think we can do better as it doesn't say what kind of confusion we're reporting (what if at some point, some other sources of confusion are added to the language?)

For the warning, I'm thinking something like
'Unexpected template literal place holder in regular string. Did you mean to use "`" quotes?'
or
'Unexpected template literal place holder in regular string. Did you mean to use a template literal?'

Maybe there's too much technical jargon, and I don't think there are questions in warning messages in core rules, so maybe not a form you like.

🚲🏠 mode on.

nzakas · 2016-07-28T15:50:41Z

what if at some point, some other sources of confusion are added to the language?

Then we can update this rule to include it, which is what I was going for. :)

The message needs to be shorter and we don't tend to give advice on how to fix the problem. We can probably get away with "Unexpected template literal placeholder in string." but not much more. We have documentation so these messages can be succinct.

jfmengels · 2016-07-28T16:18:57Z

what if at some point, some other sources of confusion are added to the language?

Then we can update this rule to include it, which is what I was going for. :)

Doesn't seem like a good idea to me as it kind of defeats the purpose of having one rule per possible error or stylistic issue. If the source of confusion is intimately related to template strings, sure, but otherwise I'd say it should be in a different rule. Therefore, I don't like no-confusing-substring. Don't have much better in mind though... Something like no-placeholder-in-string maybe?

We can probably get away with "Unexpected template literal placeholder in string."

That sounds good to me.

nzakas · 2016-07-29T16:14:38Z

Doesn't seem like a good idea to me as it kind of defeats the purpose of having one rule per possible error or stylistic issue

I'm not sure where you got this, but that's not a goal of the project. Indeed, we try to group things into rules where there are commonalities. The strict rule, for example, warns when you're missing a directive, when a directive is in the wrong place, and when a directive is unnecessary.

I'm not sure about no-placeholder-in-string because I'm not sure "placeholder" is particularly clear on its own (also, it's not a placeholder in this context, it's just a substring).

Any other team members have thoughts on naming?

ilyavolodin · 2016-07-29T17:27:26Z

no-template-in-string maybe?

ljharb · 2016-07-29T17:40:08Z

no-accidental-template-literal-notation-in-strings?

mysticatea · 2016-07-30T06:54:08Z

no-template-curly-in-string?

jfmengels · 2016-08-02T09:14:04Z

I'm not sure where you got this, but that's not a goal of the project. Indeed, we try to group things into rules where there are commonalities.

Gotcha. Thanks for the explanation.

Shall I go ahead with no-template-curly-in-string, or do you wish for more bikeshedding?

nzakas · 2016-08-02T17:29:24Z

Seems like no-template-curly-in-string has support, so I'll go along with that.

eslintbot · 2016-08-02T18:39:34Z

LGTM

jfmengels · 2016-08-02T18:41:22Z

I've updated the PR:

Changed rule name
Simplified the regex
Changed the error message

@nzakas Do you still have concerns about the content of the rule documentation?

eslintbot · 2016-08-02T20:32:12Z

LGTM

ilyavolodin · 2016-08-02T21:06:51Z

LGTM, but waiting another day for others to look

nzakas · 2016-08-03T16:27:54Z

docs/rules/no-template-curly-in-string.md

@@ -0,0 +1,33 @@
+# Warn when using template string syntax in regular strings (no-template-curly-in-string)
+


"disallow template literal placeholder syntax in regular strings"

(To fit with other rule docs)

eslintbot · 2016-08-03T18:10:02Z

LGTM

eslintbot · 2016-08-03T18:12:43Z

LGTM

eslintbot · 2016-08-03T18:13:05Z

LGTM

jfmengels · 2016-08-03T18:15:13Z

Updated with @nzakas's comments

ilyavolodin · 2016-08-09T00:57:29Z

@jfmengels Thanks so much for your contribution and being willing to work with us to get this merged!

jfmengels · 2016-08-09T06:41:26Z

Yay! 🎉 Glad to have been able to contribute :)

jquerybot added the CLA: Valid label Jul 26, 2016

jfmengels mentioned this pull request Jul 27, 2016

No template literals in normal strings sindresorhus/eslint-plugin-unicorn#15

Closed

mysticatea reviewed Jul 27, 2016
View reviewed changes

nzakas reviewed Jul 27, 2016
View reviewed changes

jfmengels changed the title ~~New: wrong-quotes-for-template-string rule (fixes #6186)~~ New: no-template-curly-in-string rule (fixes #6186) Aug 2, 2016

jfmengels added 4 commits August 2, 2016 22:15

New: wrong-quotes-for-template-string rule (fixes eslint#6186)

f49a429

Review fixes

7e7d0ed

Rename wrong-quotes-for-template-string to no-template-curly-in-string

87261a6

Simplify regex

2850ed1

jfmengels added 2 commits August 2, 2016 22:15

Update error message

769ddbf

Use const instead of var

f49d519

jfmengels force-pushed the issue-6186 branch from c9671be to f49d519 Compare August 2, 2016 20:32

nzakas reviewed Aug 3, 2016
View reviewed changes

jfmengels force-pushed the issue-6186 branch from dd611a5 to 7f9f3a1 Compare August 3, 2016 18:12

Update docs

cd947c1

jfmengels force-pushed the issue-6186 branch from 7f9f3a1 to cd947c1 Compare August 3, 2016 18:13

ilyavolodin merged commit be68f0b into eslint:master Aug 9, 2016

jfmengels deleted the issue-6186 branch August 9, 2016 09:00

eslint-deprecated bot locked and limited conversation to collaborators Feb 6, 2018

eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Feb 6, 2018

		@@ -0,0 +1,33 @@
		# Warn when using template string syntax in regular strings (no-template-curly-in-string)

New: no-template-curly-in-string rule (fixes #6186) #6767

New: no-template-curly-in-string rule (fixes #6186) #6767

Conversation

jfmengels commented Jul 26, 2016 • edited

mention-bot commented Jul 26, 2016

eslintbot commented Jul 26, 2016

platinumazure commented Jul 27, 2016

mysticatea Jul 27, 2016

Choose a reason for hiding this comment

jfmengels Jul 27, 2016

Choose a reason for hiding this comment

mysticatea Jul 27, 2016

Choose a reason for hiding this comment

mysticatea commented Jul 27, 2016

lukeapage commented Jul 27, 2016

jfmengels commented Jul 27, 2016 • edited

mysticatea commented Jul 27, 2016 • edited

eslintbot commented Jul 27, 2016

jfmengels commented Jul 27, 2016 • edited

nzakas Jul 27, 2016

Choose a reason for hiding this comment

jfmengels Jul 27, 2016

Choose a reason for hiding this comment

nzakas commented Jul 27, 2016

jfmengels commented Jul 27, 2016

nzakas commented Jul 28, 2016

jfmengels commented Jul 28, 2016 • edited

nzakas commented Jul 29, 2016

ilyavolodin commented Jul 29, 2016

ljharb commented Jul 29, 2016

mysticatea commented Jul 30, 2016

jfmengels commented Aug 2, 2016

nzakas commented Aug 2, 2016

eslintbot commented Aug 2, 2016

jfmengels commented Aug 2, 2016

eslintbot commented Aug 2, 2016

ilyavolodin commented Aug 2, 2016

nzakas Aug 3, 2016

Choose a reason for hiding this comment

eslintbot commented Aug 3, 2016

eslintbot commented Aug 3, 2016

eslintbot commented Aug 3, 2016

jfmengels commented Aug 3, 2016

ilyavolodin commented Aug 9, 2016

jfmengels commented Aug 9, 2016

jfmengels commented Jul 26, 2016 •

edited

jfmengels commented Jul 27, 2016 •

edited

mysticatea commented Jul 27, 2016 •

edited

jfmengels commented Jul 27, 2016 •

edited

jfmengels commented Jul 28, 2016 •

edited