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

Unify error wrapping #3237

Closed
aljo242 opened this issue Dec 6, 2022 · 4 comments · Fixed by #3723
Closed

Unify error wrapping #3237

aljo242 opened this issue Dec 6, 2022 · 4 comments · Fixed by #3723
Assignees
Milestone

Comments

@aljo242
Copy link
Contributor

aljo242 commented Dec 6, 2022

Context

Summary

We have inconsistent use of error wrapping throughout the codebase, mainly a mix between:

  • fmt.Errorf()
  • errors.Wrap() from pkg/errors

It makes sense to just choose one of these ways of handling errors. From the discussion on discord, there are advantages in using errors.Wrap() as it includes a stack trace to be optionally displayed to users - helpful for debugging.

pkg/errors is now archived and no longer maintained, so there is another project, https://github.com/cockroachdb/errors, that is currently maintained and is a drop-in replacement with some added functionality.

@aljo242 aljo242 added pr:need-review Change request requires peer review. type:refactor labels Dec 6, 2022
@jeronimoalbi
Copy link
Member

Are there any other benefits for the CLI of not using the standard Go errors? I mean, beyond the stack trace support.

If the only real benefit is just the stack trace support and we decide is something really helpful maybe we could just implement it ourselves instead of adding a dependency for that.

Alternatively wrapping errors coming from Go stdlib, third party packages and ignite/pkg might be enough to be a good solution.

@DimitrisJim
Copy link
Contributor

Alternatively wrapping errors coming from Go stdlib, third party packages and ignite/pkg might be enough to be a good solution.

on this, wrapcheck could be of use.

@aljo242
Copy link
Contributor Author

aljo242 commented Dec 13, 2022

So, after discussing, the team will looking to unify error handling with using fmt.Errorf() throughout the code.

This is correct right?

@jeronimoalbi
Copy link
Member

So, after discussing, the team will looking to unify error handling with using fmt.Errorf() throughout the code.

This is correct right?

As far as I know. Lets wait for @tbruyelle confirmation.

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

Successfully merging a pull request may close this issue.

5 participants