chore: upgrade .golangci.yml and workflow to v2 #6924
Conversation
run `golangci-lint fmt` Signed-off-by: Mohammed Al Sahaf <[email protected]>
mohammed90
added
under review 🧐
Signed-off-by: Mohammed Al Sahaf <[email protected]>
Signed-off-by: Mohammed Al Sahaf <[email protected]>
Signed-off-by: Mohammed Al Sahaf <[email protected]>
Signed-off-by: Mohammed Al Sahaf <[email protected]>
Signed-off-by: Mohammed Al Sahaf <[email protected]>
|
@WeidiDeng, please take a look. The linter complaints included code lines in the listener management, and those parts of the code scare me. I already introduced and fixed one bug of infinite recursion. I'm afraid I did something else 👀 |
| if !quoted && !(heredoc != heredocClosed || heredocEscaped) && | ||
| if !quoted && (heredoc == heredocClosed && !heredocEscaped) && |
There was a problem hiding this comment.
I'm not sure this one is a valuable change, I think there's a usefulness behind this condition. But maybe it's better to extract this out to a var with a clearer name. I dunno.
There was a problem hiding this comment.
I honestly struggled to read the former form 😅 How would you read it? It'll help me find a suitable variable name. The new form is read in my head as:
If not quoted, and heredoc is closed and not escaped.
| if !(quoted || btQuoted) && !(inHeredoc || heredocEscaped) && | ||
| if (!quoted && !btQuoted) && (!inHeredoc && !heredocEscaped) && |
There was a problem hiding this comment.
This one is okay I think, but both are easy enough to read I think.
|
I'm glad fmt easily fixes the imports order now, very nice addition. We should maybe add that to the contribution doc to mention how to run it |
|
The changes regarding listeners are unreasonable. Those embedded fields are explicitly referred to so it's clear whose methods or fields we are using. Because of the way caddy usage pool is designed, there are many cases of struct embedding. I see the new linter shortens many these types of embedding, and to be fair, I don't like it. Rather, I'd like for the linter to expand some of the embedding to its full form or not care about it at all. |
Noted. I'll figure out which linting rule triggers this and disable it. |
Signed-off-by: Mohammed Al Sahaf <[email protected]>
|
Embeds in several other parts are shortened when it's better to leave them as they're, such as headers and response writers. I think the linter should be run on the unmodified source before this bump. |
|
Changing it to draft until I resolve the review points. It might take me some time. |
Signed-off-by: Mohammed Al Sahaf <[email protected]>
Signed-off-by: Mohammed Al Sahaf <[email protected]>
All fixed |
There was a problem hiding this comment.
Oops, I had pending comments for like weeks ago that didn't get posted I guess. Sorry if these are late to the party. I don't even remember where I was at in the review. I'm picky about linters 😜 -- I feel like they either are WAY too opinionated, or the enforce something very specific that you want for your code base.
| break // shortcut if event not bound at all | ||
| } | ||
|
|
||
| for app.subscriptions[eventName] != nil { |
There was a problem hiding this comment.
Sometimes I like the break/return pattern because it means less indentation, or can read a little easier.
| switch subdir { | ||
| case "request_buffers": | ||
| h.RequestBuffers = size | ||
| } else if subdir == "response_buffers" { | ||
| case "response_buffers": |
There was a problem hiding this comment.
I have no idea with a linter think it's a good idea to replace 3 lines of code with 4.
| switch fromAddr.Scheme { | ||
| case "http": | ||
| fromAddr.Port = httpPort | ||
| } else if fromAddr.Scheme == "https" { | ||
| case "https": |
I also ran
golangci-lint fmt, which formats all files including tests. It found a couple issues, which is good.