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: use fmt.Errorf and fix add checks #9

Merged
merged 1 commit into from
Dec 9, 2023

Conversation

stevenh
Copy link
Contributor

@stevenh stevenh commented Nov 20, 2023

Update to use stdlib fmt.Errorf with %w for wrapping.

Use idiomatic errors:

  • Lower case
  • Describe the action not the failure
  • Remove "Error" as that's given since its an error
  • Always report the original cause
  • Quote file names using %q to its easy to identify trailing spaces

Add missing error checks.

Update to use stdlib fmt.Errorf with %w for wrapping.

Use idiomatic errors:
* Lower case
* Describe the action not the failure
* Remove "Error" as that's given since its an error
* Always report the original cause
* Quote file names using %q to its easy to identify trailing spaces

Add missing error checks.
Copy link
Owner

@ybirader ybirader left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

The majority of the changes are fine. However, I intentionally chose errors over using fmt- the reason is that fmt does not allow any context/debugging info, which means it can be hard to see the failure path.

Until Go 2, I think we should stick with errors.

Apart from that, everything looks fine.

@stevenh
Copy link
Contributor Author

stevenh commented Dec 9, 2023

To confirm my understanding by context do you mean source of the error?

If so that's why each error now uses %w which ensures the context is captured, playground example to demonstrate

@ybirader ybirader dismissed their stale review December 9, 2023 14:46

Code is fine.

Copy link
Owner

@ybirader ybirader left a comment

Choose a reason for hiding this comment

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

Thanks- completely missed the %w!

@ybirader ybirader merged commit 5a544de into ybirader:main Dec 9, 2023
3 checks passed
@stevenh stevenh deleted the chore/fmt-errorf branch December 9, 2023 20:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants