Linting

[faddat]

Gofumpt lints more tightly than gofmt + goimports, and produces more regular code.

Today I saw @mholt's tweet about a feature freeze, and wanting to improve quality. When faced with such situations and a go codebase, I typically break out golangci-lint. So, here's the PR's:

• chore: use gofumpt #5707
• chore: use gci linter #5708
• chore: Lint tests #5709
• chore: use revive linter #5712

They build off of one another, beginning with gofumpt. The one that lints the tests will be the most involved.

Goal

So, the goal of this would be to find small bugs/errors or put tests in more places, while creating a strict format for the code that coders don't need to enfoirce.

I can't say for sure that I can accomplish any style desire, but after a few years of doing extensive bughunting with golangci-lint and other tools, I can say that I can probably accomplish almost any style desire.

[mholt]

Hi @faddat -- yes, absolutely we'll be happy to welcome your expertise and review changes like this. Thank you!

A couple things to note:

• Linters can be often be too opinionated for our liking. Sometimes we just disagree and there's no real good technical basis, so we like to avoid changing working code just to change things.
• We have decided not to use tools that recommend upgrades without discretion (e.g. dependabot), as we prefer to judiciously upgrade dependencies when we feel it's a good time or when we have a specific need or when we know it's most likely safe to do so.
• Sometimes our code is intentionally linter-and-vet-unfriendly. For example, we have an extra line break somewhere because the file is a copy+paste template and we expect users to paste in their own line of code there. Or we refer to the long-broken, long-deprecated SSLv3 in code explicitly for the purpose of rejecting it at runtime and for providing helpful error messages. Or we use a weak random number generator because we need speed and not security. Or we open an if { } block just to have a comment and no actual code, because it makes it clear to readers why we're doing it that way. So if we use more linters/vet tools, we need ways to quiet their warnings around intentional violations without it being obnoxious.
• During the feature freeze, we're less inclined to make sweeping changes to the code base, i.e. changes to dozens/hundreds of files is scarier than changes to just one or two. I realize that in many cases the actual changes will be very trivial, but any structural or logical modifications -- even if theoretically equivalent -- make more more nervous and will take more time to review.

I see your PRs, and now that the busy weekend is over I can start looking at them this week. Understand if it takes some time 🙂 But since I know almost nothing about how to set up fancy linters/vet tools, I'm glad you're helping us out here! I just may have lots of nits and questions.

Thank you for the contributions!

[mholt]

Closing this issue since 2/4 changes have been merged, 1 is still open but inactive (and has merge conflicts), and I don't know if there's more to discuss at this point, BUT feel free to continue discussion if needed! Thank you!!