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

lll should not complain about long strings #11

Open
Zamiell opened this issue May 19, 2020 · 3 comments
Open

lll should not complain about long strings #11

Zamiell opened this issue May 19, 2020 · 3 comments

Comments

@Zamiell
Copy link

Zamiell commented May 19, 2020

One of, if not the most popular style guide in the world is the Airbnb JavaScript Style Guide, which represents best-practice across a lot of the JavaScript and TypeScript ecosystem.

In the style guide, it mentions:

6.2 Strings that cause the line to go over 100 characters should not be written across multiple lines using string concatenation.

Why? Broken strings are painful to work with and make code less searchable.

// bad
const errorMessage = 'This is a super long error that was thrown because \
of Batman. When you stop to think about how Batman had anything to do \
with this, you would get nowhere \
fast.';

// bad
const errorMessage = 'This is a super long error that was thrown because ' +
  'of Batman. When you stop to think about how Batman had anything to do ' +
  'with this, you would get nowhere fast.';

// good
const errorMessage = 'This is a super long error that was thrown because of Batman. When you stop to think about how Batman had anything to do with this, you would get nowhere fast.';

This policy seems sensible. With the "bad" code example above, grepping through a project for "thrown because of Batman" would return 0 results.

One other problem with using string concatenation is that realignment is tedious when you have to remove a sentence. And it introduces unnecessary Git noise. e.g.

msg := ""Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor " +
    "incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud " +
    "exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure " +
    "dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur."

-->

// We remove the second sentence, but now the entire code block has to be readjusted
msg := ""Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor " +
    "incididunt ut labore et dolore magna aliqua. Duis aute irure dolor in reprehenderit in " +
    "voluptate velit esse cillum dolore eu fugiat nulla pariatur."

In JavaScript/TypeScript land, the most popular linter is eslint, which is similar to lll in that it has a max-len rule. It has an ignoreStrings flag which is set to true by default - a sensible default.

The implementation for this is located here: https://github.com/eslint/eslint/blob/master/lib/rules/max-len.js#L314-L315
As you can see, it is extremely simple.

In Golang land, I think the practice of ignoring strings should also apply, for precisely the same reasons. I was curious to see if other big Go projects were breaking up long strings, so I took a look at Kubernetes, which is probably the biggest. I was only able to find one instance of concatenated strings. Instead, most code is written like like this.

Similar to eslint, is it possible to add an ignoreStrings flag to lll? Currently, I have gotten so fed up with adding //nolint: lll everywhere that I have stopped using the linter, but it is a great tool and I would love to be able to use it again in the future.

@orsinium
Copy link

orsinium commented Nov 6, 2020

One more reason:

Statements longer than 80 columns will be broken into sensible chunks, unless exceeding 80 columns significantly increases readability and does not hide information. <...> However, never break user-visible strings such as printk messages, because that breaks the ability to grep for them.

@agnjunio
Copy link

I also believe that Struct tags should be ignored too (they are also strings)

@mitar
Copy link

mitar commented Aug 2, 2023

I made golangci/golangci-lint#3983 to track this in golangci-lint repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants