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

Prefer-template multiline #3507

Closed
Cellule opened this Issue Aug 24, 2015 · 10 comments

Comments

Projects
None yet
7 participants
@Cellule
Contributor

Cellule commented Aug 24, 2015

Sometimes we break string in mulitple lines for lisibility like

const a = "that string can get very" +
  `long ${someVar} and we don't want it on one line`

The rule warns us to use template string when this is not needing and defeats the purpose.
Right now, there is a work around to add 2 template strings like

const a = `string1` +
  `string2`;

But I don't know if this is something we want/should keep. see #3506

@eslintbot

This comment has been minimized.

Show comment
Hide comment
@eslintbot

eslintbot Aug 24, 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!

eslintbot commented Aug 24, 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!

@eslintbot eslintbot added the triage label Aug 24, 2015

@ilyavolodin

This comment has been minimized.

Show comment
Hide comment
@ilyavolodin

ilyavolodin Aug 25, 2015

Member

@Cellule Could you paste ESLint output for the code that errors?

Member

ilyavolodin commented Aug 25, 2015

@Cellule Could you paste ESLint output for the code that errors?

@mysticatea

This comment has been minimized.

Show comment
Hide comment
@mysticatea

mysticatea Aug 25, 2015

Member

Thank you for the report.

I confirmed that.

/* eslint-env es6 */
/* eslint prefer-template:2 */
const a = "that string can get very" +
  `long ${someVar} and we don't want it on one line`;
> eslint -v
v1.2.1
> eslint test.js --no-eslintrc

test.js
  3:11  error  Unexpected string concatenation  prefer-template

✖ 1 problem (1 error, 0 warnings)

I thought this is a false positive.
Two or more string (and template literal) concatenation should be usable to make data without line breaks as keeping readability.

And we should rethink should the next pattern be warned?

let a = "this is a long string" +
    list.map(item => item.name).join(", ") +
    "and more long strings";
Member

mysticatea commented Aug 25, 2015

Thank you for the report.

I confirmed that.

/* eslint-env es6 */
/* eslint prefer-template:2 */
const a = "that string can get very" +
  `long ${someVar} and we don't want it on one line`;
> eslint -v
v1.2.1
> eslint test.js --no-eslintrc

test.js
  3:11  error  Unexpected string concatenation  prefer-template

✖ 1 problem (1 error, 0 warnings)

I thought this is a false positive.
Two or more string (and template literal) concatenation should be usable to make data without line breaks as keeping readability.

And we should rethink should the next pattern be warned?

let a = "this is a long string" +
    list.map(item => item.name).join(", ") +
    "and more long strings";
@mysticatea

This comment has been minimized.

Show comment
Hide comment
@mysticatea

mysticatea Aug 25, 2015

Member

Ah, I could write that like:

let a = `this is a long string ${
    list.map(item => item.name).join(", ")
    } and more long strings`;
Member

mysticatea commented Aug 25, 2015

Ah, I could write that like:

let a = `this is a long string ${
    list.map(item => item.name).join(", ")
    } and more long strings`;
@IanVS

This comment has been minimized.

Show comment
Hide comment
@IanVS

IanVS Aug 25, 2015

Member

For multi-line concatenation, perhaps the rule should check that the string contains a \n at the end? Because we would still want the rule to flag:

const a = "This is a multi line string.\n" +
    "It continues onto another line"

Which can be replaced by:

const a = `This is a multi line string.
    It continues onto another line.`
Member

IanVS commented Aug 25, 2015

For multi-line concatenation, perhaps the rule should check that the string contains a \n at the end? Because we would still want the rule to flag:

const a = "This is a multi line string.\n" +
    "It continues onto another line"

Which can be replaced by:

const a = `This is a multi line string.
    It continues onto another line.`
@mathieumg

This comment has been minimized.

Show comment
Hide comment
@mathieumg

mathieumg Aug 25, 2015

Contributor

@IanVS I imagine the whitespace on the second line of the second example is accidental, because the two versions are not currently equivalent.

Contributor

mathieumg commented Aug 25, 2015

@IanVS I imagine the whitespace on the second line of the second example is accidental, because the two versions are not currently equivalent.

@IanVS

This comment has been minimized.

Show comment
Hide comment
@IanVS

IanVS Aug 25, 2015

Member

Yes, you're right. You can tell I've not used them much yet. 😳

Should be?

const a = "This is a multi line string.\n" +
    "It continues onto another line"
const a = `This is a multi line string.
It continues onto another line.`

Hm, that's kinda ugly. :-/

Member

IanVS commented Aug 25, 2015

Yes, you're right. You can tell I've not used them much yet. 😳

Should be?

const a = "This is a multi line string.\n" +
    "It continues onto another line"
const a = `This is a multi line string.
It continues onto another line.`

Hm, that's kinda ugly. :-/

@mysticatea mysticatea added bug rule accepted and removed triage labels Aug 25, 2015

@mysticatea mysticatea self-assigned this Aug 25, 2015

@Cellule

This comment has been minimized.

Show comment
Hide comment
@Cellule

Cellule Aug 25, 2015

Contributor

In my opinion, if the 2 strings are not on the same line, we should not warn to use template string since it is not always preferable for lisibility

Contributor

Cellule commented Aug 25, 2015

In my opinion, if the 2 strings are not on the same line, we should not warn to use template string since it is not always preferable for lisibility

@firedev

This comment has been minimized.

Show comment
Hide comment
@firedev

firedev Jan 28, 2016

Hey, what about this

        loader: ExtractText.extract('style',
            'css?sourceMap!sass?sourceMap&outputStyle=expanded&' +
            'includePaths[]=' +
            encodeURIComponent(path.resolve(__dirname, './src')) + '&' +
            'includePaths[]=' +
            encodeURIComponent(path.resolve(__dirname,
              './src/vendor/97f4b8a9aef3-with-fixes-for-node-sass/scss')) + '&' +
            'includePaths[]=' +
            encodeURIComponent(path.resolve(__dirname, './node_modules/compass-mixins/lib'))),
      },

firedev commented Jan 28, 2016

Hey, what about this

        loader: ExtractText.extract('style',
            'css?sourceMap!sass?sourceMap&outputStyle=expanded&' +
            'includePaths[]=' +
            encodeURIComponent(path.resolve(__dirname, './src')) + '&' +
            'includePaths[]=' +
            encodeURIComponent(path.resolve(__dirname,
              './src/vendor/97f4b8a9aef3-with-fixes-for-node-sass/scss')) + '&' +
            'includePaths[]=' +
            encodeURIComponent(path.resolve(__dirname, './node_modules/compass-mixins/lib'))),
      },
@mathieumg

This comment has been minimized.

Show comment
Hide comment
@mathieumg

mathieumg Jan 28, 2016

Contributor

It seems @Cellule 's concern in #3507 (comment) wasn't addressed. I'll go on a bit of self-promotion and link you to https://github.com/mathieumg/tempura#nowhitespace which I use to build long URIs and URLs using template literals.

Contributor

mathieumg commented Jan 28, 2016

It seems @Cellule 's concern in #3507 (comment) wasn't addressed. I'll go on a bit of self-promotion and link you to https://github.com/mathieumg/tempura#nowhitespace which I use to build long URIs and URLs using template literals.

@eslint eslint bot locked and limited conversation to collaborators Feb 7, 2018

@eslint eslint bot added the archived due to age label Feb 7, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.