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

disallow templates that can be replaced with strings #3090

Closed
michaelficarra opened this Issue Jul 20, 2015 · 29 comments

Comments

Projects
None yet
9 participants
@michaelficarra
Copy link
Member

michaelficarra commented Jul 20, 2015

Untagged templates with no interpolation and no newlines end up behaving just like strings. To indicate that I'm not taking advantage of any template features, I prefer to use strings in these cases. This rule should let me know if I can replace any templates in my program with strings.

@eslintbot

This comment has been minimized.

Copy link

eslintbot commented Jul 20, 2015

Thanks for the issue! We get a lot of issues, so this message is automatically posted to each one to help you check that you've included all of the information we need to help you.

Reporting a bug? Please be sure to include:

  1. The version of ESLint you are using (run eslint -v)
  2. The source code that caused the problem
  3. The configuration you're using (for the rule or your entire config file)
  4. The actual ESLint output complete with line numbers

Requesting a new rule? Please be sure to include:

  1. The use case for the rule - what is it trying to prevent or flag?
  2. Whether the rule is trying to prevent an error or is purely stylistic
  3. Why you believe this rule is generic enough to be included

Requesting a feature? Please be sure to include:

  1. The problem you want to solve (don't mention the solution)
  2. Your take on the correct solution to problem

Including this information in your issue helps us to triage it and get you a response as quickly as possible.

Thanks!

@michaelficarra michaelficarra referenced this issue Jul 20, 2015

Merged

Merge ui #24

@gyandeeps gyandeeps added the triage label Jul 20, 2015

@mysticatea

This comment has been minimized.

Copy link
Member

mysticatea commented Jul 21, 2015

// Valid
let a = `hello, ${name}!`;
let b = `has
line break`;
let c = tag`hello`;

// Invalid
let d = `constant`;

Looks good to use together with #3014.

@mysticatea mysticatea added enhancement rule and removed triage labels Jul 21, 2015

@michaelficarra

This comment has been minimized.

Copy link
Member Author

michaelficarra commented Jul 21, 2015

Exactly.

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Jul 21, 2015

This could conflict with quotes rule when backticks are selected.

@michaelficarra

This comment has been minimized.

Copy link
Member Author

michaelficarra commented Jul 21, 2015

Yep. Good thing we allow incompatible rules.

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Jul 21, 2015

My point is that we try to avoid them wherever possible, and we definitely should think through whether the benefits outweigh the costs before doing so.

In this case, I'm not sure this rule general enough. There's no downside to using template literals for all strings, regardless of concatenation...what's the rationale for disallowing unless there are replacements?

@michaelficarra

This comment has been minimized.

Copy link
Member Author

michaelficarra commented Jul 21, 2015

I have the rationale in my original proposal:

To indicate that I'm not taking advantage of any template features, I prefer to use strings in these cases.

It is very easy for others to quickly determine that nothing funny is going on. This is why shell has single versus double quoted strings, for instance. Same thing is common in CoffeeScript, which has had interpolation in double quoted strings for years.

@gyandeeps

This comment has been minimized.

Copy link
Member

gyandeeps commented Aug 28, 2015

@ilyavolodin

This comment has been minimized.

Copy link
Member

ilyavolodin commented Aug 28, 2015

Hmm.. This is a tough one. On one hand, I agree with @michaelficarra that simple string shouldn't be using backticks, as it would make code less readable. On the other hand, it seems that the rule is pretty limited in it's use (sorry, maybe that's just my opinion, I don't use ES6, maybe if I would, I start using backticks all the time). I'm sort of between including it, or suggesting a separate plugin.

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Aug 31, 2015

I'm not questioning the rationale of the rule, just that it's appropriate for core. I don't see how we can include it given that it contradicts existing rules. The only real options are either:

  1. It's a custom rule.
  2. There's some reformulation of existing rules that allows for this behavior.
@sgtpepper43

This comment has been minimized.

Copy link

sgtpepper43 commented Sep 17, 2015

Rubocop has an almost identical rule to this for Ruby; if you set your quote preference to single quotes, it will complain when you use double quotes and don't interpolate a value. This doesn't need to be a new rule, it could easily be added to the current quotes rule as an option, similar to the "avoid-escape" option. As far as contradicting the quotes rule when set to backticks, since it'd be part of the same rule you could just ignore it, or throw an error.

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Sep 18, 2015

@sgtpepper43 can you make a concrete proposal for the change here?

@nzakas nzakas added the evaluating label Sep 18, 2015

@sgtpepper43

This comment has been minimized.

Copy link

sgtpepper43 commented Sep 18, 2015

Yeah, sure. I'll just go through the proposing a rule thing?

When the rules will warn. Include a description as well as sample code.

Taken from above:

// Valid
let a = `hello, ${name}!`;
let b = `has
line break`;
let c = tag`hello`;

// Invalid
let d = `constant`;

The rule will warn whenever backticks are used unnecessarily, ie, no interpolation is taking place, no line breaks exist, and the tag function is not being utilized.

Whether the rule prevents an error or is stylistic.

Stylistic, but with some (probably minimal) impact on performance depending on how template strings are being implemented.

Why the rule should be in the core instead of creating a custom rule.

Template strings, while not currently implemented everywhere, are part of native javascript, and so rules around it are general enough to be in the core.

Are you willing to create the rule yourself?

Possibly. There're only those three cases where backticks are useful, but I don't know how hard it would be to add those checks.

Other considerations

It would be easiest to implement as another option on the quotes rule, either by adding another option to the options array or condensing that into an options object (though that would mess up backwards compatibility). "no-unneeded-backticks" would be a good name for it. If the user has specified backticks as their preferred quote type, we can just ignore the "no-unneeded-backticks" option, since at that point they are needed. By adding it as an option, we keep it atomic, as the validity of the quotes used is only dependent on the options defined in the quotes rule.

@xjamundx

This comment has been minimized.

Copy link
Contributor

xjamundx commented Sep 18, 2015

Won't prefer-template + quotes already produce the desired effect or doesquotes just ignore template strings completely?

@michaelficarra

This comment has been minimized.

Copy link
Member Author

michaelficarra commented Sep 19, 2015

@xjamundx in what way would it do that?

@xjamundx

This comment has been minimized.

Copy link
Contributor

xjamundx commented Sep 19, 2015

I thought that the quotes rule would warn in this case (when set to double for example):

var x = "hi"; // good 
var y = `hey`; // bad
var z = 'hi'; // bad
console.log(x, y, z);

But it only shows an errors for the z declaration, not the y declaration.

So yeah, the proposal is what I thought was already happening 👍

@sgtpepper43

This comment has been minimized.

Copy link

sgtpepper43 commented Sep 21, 2015

This might not even need to be a new option, but just be the standard behavior, now that I think about it. Going from @xjamundx's example, if you've set quotes to double, it should warn when you don't use double, except in the case that double can't be used. It sounds like that's just the base of the rule. It's definitely the context of avoid-escape. The only difference is that we have prefer-template as a separate rule, so by default we have to allow backticks, or else a conflict could arise.

@IanVS

This comment has been minimized.

Copy link
Member

IanVS commented Sep 21, 2015

I was thinking the same thing, that it should be default behavior to allow backticks only in cases where they are actually necessary. I'm not sure if it would be considered breaking. It seems like more of a bug fix / refinement to me.

@michaelficarra

This comment has been minimized.

Copy link
Member Author

michaelficarra commented Sep 21, 2015

😁 That would be great!

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Sep 22, 2015

What's the proposal now?

Note: quotes "double" currently allows backticks.

@sgtpepper43

This comment has been minimized.

Copy link

sgtpepper43 commented Sep 22, 2015

It seems to now be more of a bug fix than a new rule, at this point. The expected behavior (or new expected behavior) of quotes is that it should not allow alternative quote types if the chosen quote type can be used. To get really specific, if the expression enclosed in quotes can be fully enclosed in the preferred quote type without changing functionality, it should be.

Since the only alternative to the special things that can be done within backticks is to break it out into multiple strings, even though the two following expressions are functionally the same, the expression doesn't all fit in the one string, so the use of backticks won't give a warning:

'this ' + that
`this ${that}`

While these expressions:

'this'
`this`

remain in one string, and so the use of backticks will give a warning.

The definition also allows for (by default) preferring to escape characters, as 'isn\'t and "isn't" are again both in a single string, and so the first would be preferred if quotes are set to ' (unless avoid-escape is true, obviously).

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Sep 23, 2015

I'm having a hard time rationalizing this as a bug fix vs. a new option. Can you explain?

@sgtpepper43

This comment has been minimized.

Copy link

sgtpepper43 commented Sep 23, 2015

Basically, since a backtick is just another quote type, the standard behavior of the quotes rule should warn if you use a backtick while another option is selected.

The only problem with that is that there are things you can only do with backticks (ie, template strings). To accommodate this currently, the quotes rule is just ignored when backticks are used, which shouldn't be how the rule works. A better solution is to only allow backticks when they are required.

@IanVS

This comment has been minimized.

Copy link
Member

IanVS commented Oct 30, 2015

@nzakas, what are your current thoughts on this? Do you agree with only allowing backticks which cannot be directly replaced by the preferred quote type?

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Oct 30, 2015

Yeah, I think I buy that.

@IanVS

This comment has been minimized.

Copy link
Member

IanVS commented Nov 2, 2015

Working on this.

@IanVS IanVS referenced this issue Nov 2, 2015

Open

Convert backticks #8

@IanVS

This comment has been minimized.

Copy link
Member

IanVS commented Nov 2, 2015

Blocked for the moment on sindresorhus/to-single-quotes#8.

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Dec 21, 2015

I think this is unblocked due to our new implementation, so going to give it a try.

@nzakas nzakas removed the blocked label Dec 21, 2015

nzakas added a commit that referenced this issue Dec 21, 2015

@nzakas nzakas closed this in f0b3f47 Dec 23, 2015

@IanVS

This comment has been minimized.

Copy link
Member

IanVS commented Dec 23, 2015

Awesome, thanks for picking up my slack!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.