Add a new flag to control line-length of comment lines separately. by csilvers · Pull Request #10 · walle/lll

csilvers · 2019-10-18T18:25:18Z

This allows users to have a longer line-length limit for code than for
natural-language text.

One example is a project that wants "most" code to fit in 80 columns,
but will allow the occassional long line. The best way to implement
this in lll right now is to have a longer line limit (say, 100
columns) with an understanding the code-reviewers will make sure most
lines are not over 80 columns. But we still want all comment lines to
fit within 80 columns, since there's no good reason for them not to.
This new flag allows for that.

csilvers · 2019-10-18T18:26:09Z

I forgot to say in the commit message, but I also fixed a buglet in util_test where expected and actual were reversed.

This allows users to have a longer line-length limit for code than for natural-language text. One example is a project that wants "most" code to fit in 80 columns, but will allow the occassional long line. The best way to implement this in `lll` right now is to have a longer line limit (say, 100 columns) with an understanding the code-reviewers will make sure most lines are not over 80 columns. But we still want all comment lines to fit within 80 columns, since there's no good reason for them not to. This new flag allows for that.

Zamiell · 2020-03-29T01:24:53Z

@walle Any chance of merging this?
It's a little ridiculous that the lll linter complains about code like the following:

// Get the project path
// https://stackoverflow.com/questions/18537257/how-to-get-the-directory-of-the-currently-running-file
if v, err := os.Executable(); err != nil {
	fmt.Println("Failed to get the path of the currently running executable:", err)
} else {
	projectPath = filepath.Dir(v)
}

walle

I'm sorry, I missed this PR completly :(

I think it makes sense to make this configurable, but we should probably
add support for other comment prefixes.

Also added an open question, that doesn't need to be solved in this PR,
about multi line comments.

Could you please also update the README to document the new flag(s)
and their usage? If so, please add the caveat that it only works for
one line comments right now (unless we solve the multi line issue).

The request changes feedback is because of the flag name -l which
conflicts with the previous max length flag.
As it would break the current behaviour.

cmd/lll/main.go

walle · 2020-04-05T08:48:53Z

lll.go

-func ProcessFile(w io.Writer, path string, maxLength, tabWidth int,
-	exclude *regexp.Regexp) error {
+// length is greater than maxLength.  If the line is a comment line, it
+// writes an error if the line length is greater than maxLength *or*


We could say something like: if it is identifed as a comment compared to maxCommentLength?

walle · 2020-04-05T08:50:50Z

lll.go

+// length is greater than maxLength.  If the line is a comment line, it
+// writes an error if the line length is greater than maxLength *or*
+// maxCommentLength.
+func ProcessFile(w io.Writer, path string,


Not for this PR, for later consideration.

The argument list is growing a bit too much. Maybe we should pass a config struct here instead.

walle · 2020-04-05T08:51:17Z

lll.go

-	exclude *regexp.Regexp) error {
+// Process checks all lines in the reader and writes an error if the line
+// length is greater than MaxLength.  If the line is a comment line, it
+// writes an error if the line length is greater than maxLength *or*


We could say something like: if it is identifed as a comment compared to maxCommentLength?

walle · 2020-04-05T08:57:58Z

utils.go

+
+// isComment reports whether a given line of input is a comment line.
+// It does this by checking if the line starts with `//` (excluding tabs).
+func isComment(line string) bool {


This will only work for languages that use // as the comment indicator.
Maybe we can make it configurable as well? For languages that use e.g. # for comment syntax.
Defaulting to // makes sense though.

Open question. Not sure how we should handle multiline comments, it would be nice
to add the support here if we add the flag. But we would need to look back and forward
to see if the line is part of a block comment.
Maybe that would be better done as a later step, when the line have been flagged
as too long. If we search and find that the line is part of a multiline comment we
re-evaluate it to check against the max comment length.

Ah, I didn't realize this linter was intended for use by non-go code as well. As you mention, there are already a lot of params here already, so I'm a little reluctant to add another one to identify the comment character (and make people pass it in who are using this for non-go languages). We could maybe auto-detect based on file extension, or just by analyzing the text. But that's probably too fancy.

As for multi-line, knowing how to format multi-line natural-language text -- which might have bullet-point lists, or code snippets, or other whitespace-sensitive formatting -- is very difficult. In the version I wrote for golangci-lint, I have some heuristics I use but I'm not 100% happy with them.

When I wrote the tool my idea was to use it for all the files in the
projects I was working on. But if I recall correctly the flags

GoOnly bool `arg:"-g,env,help:only check .go files"` SkipTests bool `arg:"-t,env,help:skip _test.go files"` Vendor bool `arg:"env,help:check files in vendor directory"`

where added when adding the linter to gometalinter.

I'll take a look to the parameter list and see if I can do something
better with it to be able to add more configuration stuff easily.

For the multi-line issue, yes indeed a bit difficult. But I would probably
go with a quite naive approach first and see how far it helps.

walle · 2020-04-05T09:07:50Z

@Zamiell Thanks for bumping this PR! Somehow I completly missed it :(

Zamiell · 2020-04-05T16:42:47Z

No problem, thanks for your work on this excellent linter.

Zamiell · 2020-04-05T16:45:37Z

As a side note, is it possible to just make lll ignore lines that are over X characters when there are no spaces in them? Either by default or possibly behind a new flag? (And maybe it should only apply to comments.)

csilvers · 2020-04-05T21:35:31Z

Thanks for the detailed feedback!

We wanted to use lll in the context of golangci-lint. It turns out they have their own implementation of lll so we ended up making changes there. golangci-lint gives access to the AST, so we implemented things that way, which was more annoying in some ways but made it easier to, e.g., deal with multi-line comments.

The end result ended up being quite complicated, and it's still not super-well tested within our org, but I'm happy to share it if you'd like! But these days I'm basically on childcare full time and won't be able to get to this PR for a while. Someone else who could use it is very welcome to commandeer it and push it over the finish line!

Co-Authored-By: Fredrik Wallgren <fredrik.wallgren@gmail.com>

codecov-io · 2020-04-05T21:45:08Z

Codecov Report

Merging #10 into master will increase coverage by 0.40%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #10      +/-   ##
==========================================
+ Coverage   90.76%   91.17%   +0.40%     
==========================================
  Files           2        2              
  Lines          65       68       +3     
==========================================
+ Hits           59       62       +3     
  Misses          4        4              
  Partials        2        2

Impacted Files	Coverage Δ
lll.go	`89.47% <100.00%> (ø)`
utils.go	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4438bcc...8c79f08. Read the comment docs.

walle · 2020-04-08T11:43:52Z

@Zamiell l I would add a new flag for that. Let me see if I can get some
time to add the feature soon :)

@csilvers Aha ok, I didn't know it was re-implemented in golangci-lint I'll
take a look.

And now worries about getting the PR over the finish line, I will try to
dedicate some time to it asap. And maybe add some more things :)

Thank you for your contribution!

Zamiell · 2020-04-08T13:44:41Z

@walle Thanks. To go a little bit more in depth on my suggestion, it is probably better to just copy the various flags (and the corresponding algorithms) from the most popular long-line-linter in the world, eslint max-len: https://eslint.org/docs/rules/max-len

We can see the source code for it here: https://github.com/eslint/eslint/blob/master/lib/rules/max-len.js

Specifically, relating to my suggestion, it has flags for "ignoreStrings", and "ignoreUrls", both of which are set to true by default. I think having them be true by default is a sensible idea, for both max-len and for lll!

Furthermore, if you have the time, I think it would actually be ideal if lll provided all of the flags that max-len does, as I think that people will probably expect similar functionality.

Zamiell · 2020-05-19T04:41:29Z

walle, any progress on this? I also have another request, but I'll put it in a new issue.

csilvers force-pushed the master branch from 8a6c40c to 08a6382 Compare October 18, 2019 18:27

walle requested changes Apr 5, 2020

View reviewed changes

csilvers and others added 2 commits April 5, 2020 14:43

Update cmd/lll/main.go

cca81a8

Co-Authored-By: Fredrik Wallgren <fredrik.wallgren@gmail.com>

Update cmd/lll/main.go

8c79f08

Co-Authored-By: Fredrik Wallgren <fredrik.wallgren@gmail.com>

Conversation

csilvers commented Oct 18, 2019

Uh oh!

csilvers commented Oct 18, 2019

Uh oh!

Zamiell commented Mar 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

walle left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

walle Apr 5, 2020

Choose a reason for hiding this comment

Uh oh!

walle Apr 5, 2020

Choose a reason for hiding this comment

Uh oh!

walle Apr 5, 2020

Choose a reason for hiding this comment

Uh oh!

walle Apr 5, 2020

Choose a reason for hiding this comment

Uh oh!

csilvers Apr 5, 2020

Choose a reason for hiding this comment

Uh oh!

csilvers Apr 5, 2020

Choose a reason for hiding this comment

Uh oh!

walle Apr 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

walle commented Apr 5, 2020

Uh oh!

Zamiell commented Apr 5, 2020

Uh oh!

Zamiell commented Apr 5, 2020

Uh oh!

csilvers commented Apr 5, 2020

Uh oh!

codecov-io commented Apr 5, 2020

Codecov Report

Uh oh!

walle commented Apr 8, 2020

Uh oh!

Zamiell commented Apr 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Zamiell commented May 19, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Zamiell commented Mar 29, 2020 •

edited

Loading

walle Apr 8, 2020 •

edited

Loading

Zamiell commented Apr 8, 2020 •

edited

Loading