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 nilerr linter and fix errors #5361

Merged
merged 6 commits into from
May 11, 2023

Conversation

cce
Copy link
Contributor

@cce cce commented May 5, 2023

Summary

This enables the nilerr linter that checks for cases like this:

if err != nil {
    return nil
}

Test Plan

Existing tests and new linter runs should pass.

@cce cce requested review from algorandskiy and id-ms May 5, 2023 16:22
@@ -92,5 +92,5 @@ func generateKeysForRange(ctx context.Context, startIdx uint64, endIdx uint64, k
}
keys[k] = *sigAlgo
}
return nil
return nil //nolint:nilerr // OK to not return ctx.Err()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@algoidan does this look fine?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. (Also played with it a bit)
maybe for clarity, we can change line 87:
return nil // we don't need to return the ctx error, since the other goroutine will report it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK I will push that change.. thanks!

algorandskiy
algorandskiy previously approved these changes May 5, 2023
@codecov
Copy link

codecov bot commented May 5, 2023

Codecov Report

Merging #5361 (5acd2ba) into master (9e980d3) will increase coverage by 0.01%.
The diff coverage is 40.00%.

@@            Coverage Diff             @@
##           master    #5361      +/-   ##
==========================================
+ Coverage   55.28%   55.29%   +0.01%     
==========================================
  Files         454      454              
  Lines       63772    63772              
==========================================
+ Hits        35254    35263       +9     
+ Misses      26115    26107       -8     
+ Partials     2403     2402       -1     
Impacted Files Coverage Δ
crypto/merklesignature/keysBuilder.go 74.41% <0.00%> (ø)
ledger/catchpointtracker.go 59.66% <0.00%> (+0.71%) ⬆️
libgoal/libgoal.go 2.29% <0.00%> (ø)
data/transactions/logic/eval.go 91.66% <100.00%> (ø)
ledger/acctonline.go 80.13% <100.00%> (+0.45%) ⬆️

... and 15 files with indirect coverage changes

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

tzaffi
tzaffi previously approved these changes May 8, 2023
@cce cce requested review from winder and bbroder-algo May 11, 2023 17:27
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.

5 participants