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

chore: properly propagate fmt.Errorf errors + use errors.New #16537

Merged
merged 4 commits into from
Jun 22, 2023

Conversation

odeke-em
Copy link
Collaborator

Changes usages of fmt.Errorf that tried to propagate errors using format specifiers "%s" or "%v", and unnecessarily invoked err.Error() to pass into "%s"
While here, replaced some instances of

fmt.Errorf(string)

with

errors.New(string)

Fixes #16536

@odeke-em odeke-em requested a review from a team as a code owner June 14, 2023 05:50
@github-actions github-actions bot added C:CLI C:Confix Issues and PR related to Confix C:Keys Keybase, KMS and HSMs C:Rosetta Issues and PR related to Rosetta C:x/auth C:x/genutil genutil module issues C:x/gov C:x/params C:x/staking C:x/upgrade labels Jun 14, 2023
@github-actions

This comment has been minimized.

@odeke-em odeke-em force-pushed the all-propagate+properly-wrap-errors-in-fmt.Errorf branch from 811db3e to c49f24a Compare June 14, 2023 05:52
Copy link
Member

@julienrbrt julienrbrt left a comment

Choose a reason for hiding this comment

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

lgtm, but build is failing.

types/dec_coin.go Outdated Show resolved Hide resolved
CHANGELOG.md Outdated
@@ -40,7 +40,8 @@ Ref: https://keepachangelog.com/en/1.0.0/

### Improvements

* (all) [#16497](https://github.com/cosmos/cosmos-sdk/pull/16497) Removed all exported vestiges of `sdk.MustSortJSON` and `sdk.SortJSON`.
* (all) [#16537](https://github.com/cosmos/cosmos-sdk/pull/16497) Removed all exported vestiges of `sdk.MustSortJSON` and `sdk.SortJSON`.
* (all) [#16497](https://github.com/cosmos/cosmos-sdk/pull/16537) Properly propagated fmt.Errorf errors + using errors.New where appropriate.
Copy link
Member

Choose a reason for hiding this comment

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

The pr numbers seem to be inverted

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed!

Copy link
Member

Choose a reason for hiding this comment

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

I still they are still inverted.

@julienrbrt julienrbrt changed the title all: properly propagate fmt.Errorf errors + use errors.New chore: properly propagate fmt.Errorf errors + use errors.New Jun 14, 2023
@odeke-em odeke-em force-pushed the all-propagate+properly-wrap-errors-in-fmt.Errorf branch 2 times, most recently from d1a7b10 to 484b30e Compare June 14, 2023 23:03
@odeke-em odeke-em enabled auto-merge June 14, 2023 23:05
@odeke-em
Copy link
Collaborator Author

Thanks for the reviews @julienrbrt and @alexanderbez!

@tac0turtle
Copy link
Member

one more linting issue then we are good. Thanks for sticking through this

Changes usages of fmt.Errorf that tried to propagate
errors using format specifiers "%s" or "%v", and unnecessarily
invoked err.Error() to pass into "%s"
While here, replaced some instances of

    fmt.Errorf(string)

with

    errors.New(string)

Fixes #16536
@odeke-em odeke-em force-pushed the all-propagate+properly-wrap-errors-in-fmt.Errorf branch from 484b30e to fe41652 Compare June 15, 2023 20:14
@odeke-em
Copy link
Collaborator Author

Thanks for the reviews @tac0turtle @facundomedica @julienrbrt @alexanderbez. All good to go shortly!

@tac0turtle
Copy link
Member

the test failure need to be button upped

@odeke-em odeke-em added this pull request to the merge queue Jun 22, 2023
Merged via the queue into main with commit 99a570a Jun 22, 2023
52 checks passed
@odeke-em odeke-em deleted the all-propagate+properly-wrap-errors-in-fmt.Errorf branch June 22, 2023 09:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:CLI C:Confix Issues and PR related to Confix C:Keys Keybase, KMS and HSMs C:Rosetta Issues and PR related to Rosetta C:x/auth C:x/genutil genutil module issues C:x/gov C:x/params C:x/staking C:x/upgrade
Projects
None yet
Development

Successfully merging this pull request may close these issues.

all: ensure propagated errors in fmt.Errorf errors are wrapped with %w and not %s nor %v
5 participants