Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lint: enable go vet printf format arg checks for logging #4679

Merged
merged 3 commits into from
Oct 25, 2022

Conversation

cce
Copy link
Contributor

@cce cce commented Oct 20, 2022

Summary

While reviewing #4149 I noticed a %d where %s should be and remembered we can enable go vet checks on custom logging functions by changing our linter settings. This fixes all the logging format string errors (in all files, going back before our configured new-from-rev commit) and enables the go vet configuration change.

Test Plan

Code should be lint clean, only affects logging.

@codecov
Copy link

codecov bot commented Oct 20, 2022

Codecov Report

Merging #4679 (1ce07d6) into master (b29d7ff) will decrease coverage by 0.01%.
The diff coverage is 10.52%.

@@            Coverage Diff             @@
##           master    #4679      +/-   ##
==========================================
- Coverage   54.38%   54.37%   -0.02%     
==========================================
  Files         403      403              
  Lines       51928    51928              
==========================================
- Hits        28243    28234       -9     
- Misses      21308    21313       +5     
- Partials     2377     2381       +4     
Impacted Files Coverage Δ
agreement/cadaver.go 58.33% <0.00%> (ø)
daemon/algod/api/server/v2/handlers.go 0.00% <0.00%> (ø)
data/account/registeryDbOps.go 73.84% <0.00%> (ø)
data/accountManager.go 71.25% <0.00%> (ø)
ledger/acctonline.go 77.60% <0.00%> (-0.53%) ⬇️
ledger/tracker.go 74.89% <0.00%> (ø)
network/wsNetwork.go 65.80% <0.00%> (+0.27%) ⬆️
network/wsPeer.go 68.44% <0.00%> (+1.94%) ⬆️
node/node.go 4.40% <0.00%> (ø)
rpcs/txService.go 54.25% <0.00%> (ø)
... and 16 more

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

algorandskiy
algorandskiy previously approved these changes Oct 20, 2022
@@ -69,9 +104,9 @@ issues:
- path: _test\.go
linters:
- errcheck
- gofmt
# - gofmt
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why disabled?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

they're actually being enabled.. this is an exclude list, so the linters being commented out here are the only ones that will run on test code

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why comment them out instead of removing them?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe @algojack set it up that way so it was more clear what linters were being run on tests — this way you can see that gofmt, govet, and revive are the linters we want to run on tests

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm ok with deleting comments, just wanted to make sure that we didn't accidentally forget to ignore that excluded path on any linters. If we keep comments we can see that we enabled them there on purpose vs just forgetting to exclude them. Doesn't matter much.

Copy link
Contributor

@winder winder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd remove the comments, other than that LGTM.

Copy link
Contributor

@algojack algojack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants