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

Report errors in Close() defer functions #11785

Closed
4 tasks
robert-zaremba opened this issue Apr 26, 2022 · 3 comments · Fixed by #13619
Closed
4 tasks

Report errors in Close() defer functions #11785

robert-zaremba opened this issue Apr 26, 2022 · 3 comments · Fixed by #13619
Labels
C:errors good first issue T: Dev UX UX for SDK developers (i.e. how to call our code) Type: QA Quality Assurance

Comments

@robert-zaremba
Copy link
Collaborator

Description

Very often we ignore errors, or omit them in a defer functions, eg:

defer file.Close()

Proposal

Create a wrapper function:

func logError(logger, f func() error) {
    if err := f(); err != nil {
        logger.Error(err)
    }
}
...

defer logError(ctx.Logger(), file.Close)

For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@robert-zaremba robert-zaremba added T: Dev UX UX for SDK developers (i.e. how to call our code) Type: QA Quality Assurance C:errors labels Apr 26, 2022
@alexanderbez
Copy link
Contributor

ACK. Would be in favor of calling it LogDeferred

@leoflalv
Copy link
Contributor

Hi, I am new to this repo and would like to take this task, can I? I just have a question, should it be part of this task to add the new helper in the necessary places?

@robert-zaremba
Copy link
Collaborator Author

@leoflalv sure. I propose you to firstly create a small PR by updating only one module. Maybe x/bank or /client.

@mergify mergify bot closed this as completed in #13619 Oct 26, 2022
mergify bot pushed a commit that referenced this issue Oct 26, 2022
## Description

Closes: #11785

A new helper function called `LogDeferred` was added. This function it's used to report errors in `defer` section. It was added in `defer` statements of `x/bank` folder.

### Author Checklist

I have...

- [x] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [x] added `!` to the type prefix if API or client breaking change
- [x] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#pr-targeting))
- [x] provided a link to the relevant issue or specification
- [x] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/main/docs/building-modules)
- [x] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#testing)
- [x] added a changelog entry to `CHANGELOG.md`
- [x] included comments for [documenting Go code](https://blog.golang.org/godoc)
- [x] updated the relevant documentation or specification
- [x] reviewed "Files changed" and left comments if necessary
- [x] confirmed all CI checks have passed

### Reviewers Checklist

*All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.*

I have...

- [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] confirmed `!` in the type prefix if API or client breaking change
- [ ] confirmed all author checklist items have been addressed 
- [ ] reviewed state machine logic
- [ ] reviewed API design and naming
- [ ] reviewed documentation is accurate
- [ ] reviewed tests and test coverage
- [ ] manually tested (if applicable)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:errors good first issue T: Dev UX UX for SDK developers (i.e. how to call our code) Type: QA Quality Assurance
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants