chore: lint: update linter settings, fix lint errors

[rvagg]

Ref: #11967

This isn't complete, I just banged through a few iterations of it to see the kinds of changes we're being asked to make. I think a better approach here is to add some exclusion error regexes. Most of the issues fall into two categories:

1. Named but unused parameters
2. Acronym capitalisation - Api -> API, Http -> HTTP, Url -> URL, Id -> ID

The latter suck in particular because a lot of them are on API methods. I've started adding explicit exclusions for those in here, but we could opt for a global exclusion (minimal might be something like var-naming: var [A-Z].+ should be to capture public variables).

Or we could just hammer it with exclusions to minimise the diff in this PR in particular, even turn off var-naming entirely.

[magik6k]

Acronym capitalisation - Api -> API, Http -> HTTP, Url -> URL, Id -> ID

When we were setting up the initial linters we very explicitly disabled all linters which were based on pure opinion and weren't providing any value other than aesthetics based on the linters author taste.

Linters which complain "oh no your variable should be ABC instead of Abc" are purely annoying, often wrong, and just slow down development, where often you'd just push a commit, now you have to push a commit, pray that the linter passes with no annoynig-positives, and possibly/likely go back to the PR to push some fixes which don't fix anything important

Tldr most opinion-based linters imo don't make sense unless most of your team is junior devs

[rvagg]

I think I agree, I was getting slightly angry at it telling me that HttpServer must be HTTPServer. I'll work on some exclusions, maybe turn off var-naming entirely.

[ZenGround0]

but we could opt for a global exclusion (minimal might be something like var-naming: var [A-Z].+ should be to capture public variables).

I like the sound of a global exclusion for this

[snadrus]

I'm alright turning off both of these things. Because these are not actual problems, but style matters

[rvagg]

I re-did this PR from scratch, and revisited the whole config. Here's the choices I've made:

• Removed the deprecated linters golint, varcheck, deadcode and scopelint, replacing with their equivalents exportloopref and revive
• Removed golint specific path exclusions
• Remove goconst - it had two separate exclusions that meant it wasn't being used anyway, I think we're experienced enough to not need this
• All message exclusions are more explicit regexes now, they're harder to find ways around (harder to subvert) and it's much clearer in the config the exclusion decisions we're making
• I decided to ditch all exclusions to unchecked errors except for 3 explicit ones in node/repo/fsrepo.go and 2 in paychmgr/simple.go. This mostly required adding _ = in place of the explicit exclusions and logger.SetLogLevel had a bunch of unchecked but we have far more _ = in the repo so I'm just making that consistent.
• A bunch of minor changes that are obvious in the diff
• I decided not to exclude most of the new empty-block errors, but instead I ended up rewriting some logic. I get why some people like empty blocks for comments, but I don't believe they ultimately aid in clarity. Reviewers please pay careful attention to my changes to make sure I didn't change any logic; some are non-trivial.

[magik6k]

Only looked at the SP side of things, but looks good to me

[ribasushi]

Read through the diff - everything seems correct functionally.

Left 2 comments where things were most jarring.

[ribasushi]

@rvagg in the final diff-set this comment got lost
Unsure how important for understanding the rest of the stuff, this is your-ish area

[rvagg]

good catch, I'd rather not lose it, #12007