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

Reverse rule on string concatenation for long lines #995

Merged
merged 1 commit into from
Aug 4, 2016

Conversation

lencioni
Copy link
Contributor

@lencioni lencioni commented Aug 4, 2016

Broken and concatenated long strings are painful to work with and
produce less readable and searchable code. I think we should reverse
this rule.

Unfortunately, the max-len rule currently does not allow for this, but
there is currently a proposal to add an option to ESLint's max-len rule
that would allow for strings to be ignored.

eslint/eslint#5805

There have also been discussions around performance of string
concatenation (#40), but I
don't think that is very relevant here so I removed the links to them.

@ljharb
Copy link
Collaborator

ljharb commented Aug 4, 2016

LGTM

Broken and concatenated long strings are painful to work with and
produce less readable and searchable code. I think we should reverse
this rule.

Unfortunately, the max-len rule currently does not allow for this, but
there is currently a proposal to add an option to ESLint's max-len rule
that would allow for strings to be ignored.

  eslint/eslint#5805

There have also been discussions around performance of string
concatenation (#40), but I
don't think that is very relevant here so I removed the links to them.
@lencioni lencioni merged commit dc7e7e7 into master Aug 4, 2016
@lencioni lencioni deleted the string-concatenation branch August 4, 2016 16:53
@chrisngobanh
Copy link
Contributor

While you bring up a good point about it being less searchable, I disagree with your opinion that it makes it easier to read. I always make sure that my line lengths are within the 100 character limit because it's obnoxious to have to scroll to the right in my text editor to read a line of code.

I propose that we go forth with adding the ignoreStrings option to the max-len rule when it becomes available, while changing this rule to say something along the lines of "we don't care if you want to concatenate your strings or not as long as you concatenate them correctly if you choose to do so". In other words, we should change this rule to be less strict with how we want the developer to format their strings.

@ljharb
Copy link
Collaborator

ljharb commented Aug 22, 2016

@chrisngobanh the string's contents aren't something that needs to be read - strings are data, a black box. The code is what matters.

We'll definitely add that option when it becomes available, but strings broken across lines are very hard to read, so I don't think we should change the guide in that manner.

@chrisngobanh
Copy link
Contributor

@ljharb We need to update 18.12

@ljharb
Copy link
Collaborator

ljharb commented Aug 23, 2016

@chrisngobanh see #1027

hibearpanda pushed a commit to 15Prospects/javascript that referenced this pull request Jan 22, 2017
Broken and concatenated long strings are painful to work with and
produce less readable and searchable code. I think we should reverse
this rule.

Unfortunately, the max-len rule currently does not allow for this, but
there is currently a proposal to add an option to ESLint's max-len rule
that would allow for strings to be ignored.

  eslint/eslint#5805

There have also been discussions around performance of string
concatenation (airbnb#40), but I
don't think that is very relevant here so I removed the links to them.
jaylaw81 pushed a commit to appirio-digital/ads-best-practices that referenced this pull request Sep 19, 2017
Broken and concatenated long strings are painful to work with and
produce less readable and searchable code. I think we should reverse
this rule.

Unfortunately, the max-len rule currently does not allow for this, but
there is currently a proposal to add an option to ESLint's max-len rule
that would allow for strings to be ignored.

  eslint/eslint#5805

There have also been discussions around performance of string
concatenation (airbnb#40), but I
don't think that is very relevant here so I removed the links to them.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants