Quality: Switch from golint to golangci-lint.

[winder]

Summary

Future tasks:

• Install custom partitiontest linter.
• Move logic from .github/workflows/reviewdog.yml into Makefile
• push back new-from-rev

Test Plan

CI passes.

[codecov]

Codecov Report

Merging #4418 (f40ca28) into master (2848e37) will increase coverage by 0.00%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #4418   +/-   ##
=======================================
  Coverage   55.25%   55.25%           
=======================================
  Files         398      398           
  Lines       50148    50148           
=======================================
+ Hits        27709    27711    +2     
- Misses      20127    20128    +1     
+ Partials     2312     2309    -3     

Impacted Files	Coverage Δ
ledger/tracker.go	73.93% <0.00%> (-0.86%)	⬇️
ledger/acctupdates.go	69.29% <0.00%> (-0.60%)	⬇️
network/wsPeer.go	70.41% <0.00%> (-0.28%)	⬇️
catchup/service.go	69.38% <0.00%> (+0.49%)	⬆️
catchup/peerSelector.go	100.00% <0.00%> (+1.04%)	⬆️
ledger/blockqueue.go	88.50% <0.00%> (+2.87%)	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[jannotti]

I'm no expert here, but this looks good to me.

[jannotti]

What is the new checkpoint, and how was it chosen?

Is there a convenient way to run all of the checks against a single file? I'd occasionally like to clean up some older code, if I'm making a lot of changes in an area.

[winder]

It's the most recent PR. We have a number of errcheck and ineffassign failures since eb019291beed556ec6ac1ceb4a15114ce4df0c57.

There are a lot of options for golangci-lint run, but I don't see one to specifically override the rev. I think it would be good to work backwards over time, but that doesn't really help you target specific packages.

[jannotti]

Or, perhaps we can run it twice (I'm assuming the caching makes that fast), once with the old config on the old checkpoint, and once with more stringent checks on a more recent one.

It would be very cool if you could associate a rev with each checker, when you decide as a group to adopt a new policy.

(Leaving this comment, but I suppose it makes no sense to do that first thing. If we know we have no problems between commits A and B, then running the more stringent check on code after the later commit is sufficient.)

[jannotti]

--new-from-rev REV seems to do it at the CLI

and this is interesting, but maybe it would be a big pain on large files.
--whole-files Show issues in any part of update files (requires new-from-rev or new-from-patch)

[winder]

If it's a common check, you could maintain additional config files and pass them in with -c. There is an exclude directory option, so you could filter that way.

There are really a huge number of options: https://github.com/golangci/golangci-lint/blob/master/.golangci.reference.yml

[cce]

I have a branch with most of these fixed I think

[cce]

that is #4241

[cce]

I have always hoped we will move this commit pointer backwards, never forwards..

[jannotti]

Now that #4241 is merged, can you rebase onto it, and see if we can move leave the rev as is? (Or back it up?)

[winder]

@jannotti thanks for the heads up, I missed that. I reverted the new-from-rev change and all looks good.

[gmalouf]

I think linter can be a GitHub action

…

On Tue, Aug 16, 2022 at 9:05 AM John Jannotti ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In .golangci.yml <#4418 (comment)>: > @@ -17,7 +17,9 @@ severity: issues: # use these new lint checks on code since #2574 - new-from-rev: eb01929 + #new-from-rev: eb01929 + # use stricter enforcement since ##4410 + new-from-rev: fd488f8 --new-from-rev REV seems to do it at the CLI — Reply to this email directly, view it on GitHub <#4418 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAHP3U7SCF3BINHZ2O6DKJLVZOGZPANCNFSM56VQSF3Q> . You are receiving this because your review was requested.Message ID: ***@***.***>

[winder]

I think linter can be a GitHub action

It's also a github action, yes. This change aligns development with CI.

[cce]

Please don't move new-from-rev, just merge #4241