lll: Ignore long URLs, strings, and struct tags

[mitar]

Your feature request related to a problem? Please describe.

There are upstream issues in lll about those with very good arguments:

• URLs are also reported as too long walle/lll#12
• lll should not complain about long strings walle/lll#11

Describe the solution you'd like.

I think ignoring those should at least be possible through options, but I would even claim that those should be defaults. There is simply no easy way to wrap URLs and struct tags. And wrapping long strings just makes it less readable.

Describe alternatives you've considered.

I currently have to manually add lint ignore pragmas in those cases.

Additional context.

Upstream lll repository seems not maintained. So I am opening an issue here. There is a precedent for this, see upstream issue walle/lll#15 and fix here #3288.

[ldez]

lll is based on text content, there is no AST, so this linter doesn't understand the Go syntax.

[mitar]

I mean, if it simply does not count long strings of any type to the line length, I think this is then enough? So it just counts number of characters in a line which are outside of any strings. That handles struct tags as well.

[ldez]

a string is a language-related notion.

[ldez]

Theoretically and historically, lll is a linter that should limit line length to fit the length of a screen.

In theory, this linter should not care about language-related notions except if there is a compilation problem (like import) because the target is the length of the screen.

For me, the 2 issues (URL and string) are not related at all: one is about comment, and one is about string. And string and struct tags are 2 differents topics.

The following example (split of the URL) results in a valid link inside the render of godoc:

// main runs the app.
// https://github.com/walle/lll/blob/4438bccd245f
// 7e87a5a69023625f01e7035c05c0/utils.go#L15
func main() {

I know it feels weird but godoc trims \n.

I'm able to create something to ignore comments starting with URL but this doesn't follow the screen rule.

In the modern world, the screen length is not really a problem: screens are no longer limited to 80 characters and IDEs have line wrapping.

My opinion and it's just my opinion, the rule behind lll is something from the past and not really relevant in the modern world. I prefer semantic line breaks inside documentation and comments.

[ldez]

About the string, for me, the 2 points of view (not splitting or splitting) illustrate the problem behind lll: never splitting a long string is not a good rule, and always splitting a long string is not a good rule.
The good rule is to split or not depending on the context, a linter cannot help because a human must take a decision.

The current approach, limiting by default and using nolint to ignore, is the best approach because it relies on a human decision.

---

About struct tag, this is a language-related notion, it's not manageable without the AST.
The best solution is to use nolint directive in this context.

[ldez]

I created PR #3986 to ignore comments with only an URL.

But as explained in my previous comments, the other topics (string and struct tag) will not be handled.

[mitar]

Not sure why lll would not also ignore lines with backtick strings in them. You can have a regular expression to do that. And that would cover both struct tags and long strings. That addresses I think your comments/views as well: let's have backtick strings be allowed and not counted towards the length of the line while regular ones do count. Users can then decide which one to use as well as a way to communicate the context.

About struct tag, this is a language-related notion, it's not manageable without the AST.
The best solution is to use nolint directive in this context.

I disagree here a lot. We are talking about Go linter - lll. Not some general line length linter for general files. I think this is the main difference here. I would expect that all linters in golangci-lint know and understand Go and specially handle Go specifics (imports, struct tags, etc.). There is simply no case where you would not want to disable struct tags line length. So making this something that all users of golangci-lint have to set to ignore is simply ridiculous.

Now you are saying that this is only doable with AST. Then maybe the linter should also use AST to do its work. If it is too simple how it currently operates it is too simple. That is the beauty of Go, that AST is easy available and that you can do all kinds of linters. Otherwise I can just have vim rewrap text for me without understanding the language. But linter becomes useful because it can understand the language.

But in my view simply removing all backtick strings before counting the line length would fix the issue in my view. But if you think strings should not be touched, then we need AST to solve the struct tags.

[bombsimon]

I think struct tags is actually a valid thing to filter out because that's the only case that's not even supported by the language (there's no way to split over multiple lines) but I don't think it's that simple without the AST since you can put one or more comments anywhere in such line. This is valid Go code:

type A struct {
    /* Name */ Name /* Type */ int /* Tag */ `tag:"value"`
}

And I don't like the idea of false positives or false negatives which would occur by ignore such (edge) cases.

Ignoring lines with a backtick (`) is somewhat the same.

var x = `very [...] long line` // Would be ignored

var y = `very
[...] long
line` // Would yield error on the second line if too long

I'm not sure if I think #3986 is a good idea, it looks like a very specific use case and might just be confusing to allow a single URL but not something like the comment suggests of See: [url].

Not really related by just to confirm this is a personal and opinionated topic, I think lll is a reasonable rule to have even with modern monitors. I prefer wrapped lines because it's easier for me to read and it also makes me able to read code even if I use multiple splits in my editors (which I almost always do).

Personally I prefer golines that will wrap long lines of Go code where possible but ignore everything that can't be wrapped such as comments and strings. Also might be worth giving the feature in gofumpt a tryand follow the development of line wrapping for that formatter. I think both of those tools is a better and more future proof direction rather than trying to adjust lll.

[ldez]

I think struct tags are actually a valid thing to filter out

I didn't say the opposite but this issue is trying to mix 3 different topics.
I think that comment with only a URL is easy to handle and doesn't require complex regular expression.

I don't want to create a magic pattern (See: ).

Your syntax is not complete:

type A struct {
    /* Name */ Name /* Type */ int /* Tag */ `tag:"value"`
}

This case (embed type) is also valid:

type A struct {
    /* Type */ Foo /* Tag */ `tag:"value"`
}

And to detect that without a false positive, you have to know that you are in a struct.

Ignoring lines with a backtick (`) is somewhat the same.

You need to know when the block starts and ends by just reading line by line, it's not trivial and you will need to understand what is a simple string and what is a regexp.

Also when using AST the line doesn't exist, so the length of a line is not available.

Not really related by just to confirm this is a personal and opinionated topic, I think lll is a reasonable rule to have even with modern monitors.

My opinion is that a rule simply based on length is not relevant, the split must be based on semantics and not length.

I created a PR despite my opinion about lll because I don't have to follow my opinion here.

---

I think both of those tools is a better and more future proof direction rather than trying to adjust lll.

We can define a new philosophy behind lll by ignoring the historical link with screen size but we have to be clear about this philosophy.

But I totally agree with you: lll is too naive, creating a new linter/rule is a better way.

[mitar]

@ldez Thank you for being open in changing lll in ways you personally do not necessary agree with.

I am not familiar with AST internals and how hard it is to do line length counting using AST, but I could imagine an ugly tool which would simply reformat a Go file and remove all struct tags. And then count line lengths in the resulting file. Any line which is too long in those reformatted file would be reported as too long in the original file. Reformatted file is just a temporary (or even in-memory) file.

My main issue is with struct tags where once you start using tagliatelle you get very long struct tags.

lll is too naive

Maybe such a internal preprocessing step (of removing struct tags) could be done for lll? Then the logic of lll itself could stay as it is.

[ldez]

Using AST means no line: because AST is an abstraction of the code not of the line.

So creating a linter on this topic based on AST is not trivial because it requires handling several unrelated things at the same time.

[mitar]

because AST is an abstraction of the code not of the line.

Sure, but every token in AST tree has line number, line offset, and line end. So you just go over all tokens and check for all tokens where they end. If they end after the max line length position, you issue lll warning for that line? And you can then skip struct tag nodes while walking the AST?

[ldez]

just an example of the complexity:

const aaa = `foofoofoofoofoofoo` + "`foofoofoofoofoo`" + `foofoofoofoofoofoo`