chore: Lint tests

[faddat]

THis PR lints the tests in caddy, too.

• adds thelper linter (gives additional context in your editor when tests fail)
• adds revive linter
• lints the entire codebase
• adds nolintlint so that nolint statements don't hang around where they're not needed

[CLAassistant]

All committers have signed the CLA.

[mholt]

Hmmm. I disagree with the linter in some places here. I think a field name like publicKey is perfect because it's the final result of processing the PublicKeys field after provisioning from config.

[faddat]

@mholt -- indeed, I disagreed with it too :).

Please lmk if I made any changes based on its disagreeable choices.

I basically finished this one out by disabling a few things:

• confusing-naming
• unhandled-error
• import-shadowing

I'f you'd like any of them on, please let me know that too :)

Ah, this one still has a few traces of unused-parameter in it, I'll get those out too.

[faddat]

@mholt -- those _'s were kinda the result of a misconfiguration.

I think that their clarity (eg: we don't do anything with this) is less-good than the self-documenting code. I did this on ibc-go a while ago and ended up having to seek out names a bunch.

Overall, I wanted to let the maintainers here know, the idea behind this set of PR's was to make writing code easier by transferring as much of the formatting work to a machine as possible. In the long run I'd recommend progressively tightening the linters, because you can catch small/easy issues (that can be maddening) that way, and also ensure that the code is really rigorously formatted.

The reason I came to do this now, was the tweet about the feature freeze -- that's pretty much a perfect time to do this kind of work, because when linting we don't want to change what the code does at all (except maybe adding some tests, the linters are pretty good at finding places where we could add additional tests).

Ah, and about linting the tests -- so that's not default behavior for golangci-lint but it probably should be. Once you're linting all of the code including the tests, you can get good consistency on the whole codebase, really fast.

You can think of the linter configuration, as the thing that handles all of the formatting of the code, and in the future, you can use it to make sometimes surprisingly granular changes even to small stuff.

[francislavoie]

Thanks for the review comments @AnomalRoil, appreciated!

This PR will need rebasing, it's fallen a bit behind.

[mholt]

Yes, thank you from me too, @AnomalRoil !

But I also think that if you're replacing named return with explicit returns, you might as well drop the named returns most of the time. (Despite @mholt clearly loving these named returns, I think it doesn't help with readability of long functions, so it's probably better to have explicit return values.)

If I read you right, I think I agree. Named returns can be very elegant sometimes and even help improve correctness / reduce error-proneness, but if most/all of the return statements end up using variable names, the names should be removed from the return signature.

@faddat Any interest in finishing this up? Apologies if it's overwhelming. That's just what happens sometimes with big changes :sweat_smile: We appreciate it!

[mholt]

Closing due to inactivity -- however, we'd be open to finishing this when you are @faddat (or anyone else who would like to pick this up)!

Maybe it would be easier to break it down into smaller changes if possible.