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: add unnecessary conversions linter to automated PR advice #4710

Merged
merged 1 commit into from
Nov 2, 2022

Conversation

cce
Copy link
Contributor

@cce cce commented Oct 31, 2022

Summary

This adds the unconvert linter to the .golangci-warnings.yml configuration, which is used by Github Actions & reviewdog to provide automated advice on new lines added that provides linter output, but does not block a PR.

Test Plan

No change besides linter settings.

@codecov
Copy link

codecov bot commented Oct 31, 2022

Codecov Report

Merging #4710 (c599689) into master (ad08f74) will decrease coverage by 0.06%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #4710      +/-   ##
==========================================
- Coverage   54.49%   54.42%   -0.07%     
==========================================
  Files         407      407              
  Lines       52389    52389              
==========================================
- Hits        28548    28513      -35     
- Misses      21455    21481      +26     
- Partials     2386     2395       +9     
Impacted Files Coverage Δ
ledger/roundlru.go 90.56% <0.00%> (-5.67%) ⬇️
ledger/blockqueue.go 84.48% <0.00%> (-4.03%) ⬇️
agreement/cryptoVerifier.go 67.60% <0.00%> (-2.12%) ⬇️
agreement/proposalManager.go 96.07% <0.00%> (-1.97%) ⬇️
network/wsPeer.go 66.50% <0.00%> (-1.95%) ⬇️
data/transactions/verify/txn.go 76.19% <0.00%> (-0.96%) ⬇️
ledger/catchpointtracker.go 62.10% <0.00%> (-0.79%) ⬇️
ledger/testing/randomAccounts.go 55.59% <0.00%> (-0.63%) ⬇️
ledger/acctupdates.go 69.59% <0.00%> (-0.60%) ⬇️
network/wsNetwork.go 65.34% <0.00%> (-0.19%) ⬇️
... and 3 more

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

Copy link
Contributor

@michaeldiamant michaeldiamant left a comment

Choose a reason for hiding this comment

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

@cce Seems reasonable to me.

I'd also be on-board to make unnecessary conversions an error. It's possible an unnecessary conversion becomes necessary through various future changes. Depending on the situation, a conversion may not be desired behavior.

@cce
Copy link
Contributor Author

cce commented Nov 2, 2022

Sure! If you want to do that it is a nice soothing sudoku-like game to go in and fix them all and get them lint clean in another PR.. I had intended to use the warning-level linter for introducing potentially controversial linter opinions like this

@cce cce merged commit fa77937 into algorand:master Nov 2, 2022
@cce cce deleted the unconvert-warnings branch November 2, 2022 14:23
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.

2 participants