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

Improve error handling in RPC package #187

Merged
merged 4 commits into from
Nov 30, 2021
Merged

Conversation

lthibault
Copy link
Collaborator

On several occasions I have found myself wanting to inspect RPC exceptions to control execution flow, in particular via functions such as errors.Is or errors.As. Currently, errors are collapsed when composed, making them difficult to inspect.

This PR is an incremental attempt at improving error composition in the code base. It exports the errors.Error type, enhances it with an Unwrap() method to make it compatible with the standard library errors package, and replaces %v formatting directives with %w and fmt.Errorf.

In addition to making errors more interpretable for the user, the resulting code is much more readable IMHO.

internal/errors/errors.go Show resolved Hide resolved
go.mod Show resolved Hide resolved
@lthibault lthibault self-assigned this Nov 1, 2021
@lthibault lthibault force-pushed the enhancement/wrap-errors-rpc branch from 8587408 to ffa3449 Compare November 29, 2021 14:18
@lthibault lthibault requested a review from zenhack November 29, 2021 14:27
@lthibault
Copy link
Collaborator Author

@zenhack I just rebased against main and double-checked that the errors string was properly formatted. Are you ok to merge this?

@lthibault
Copy link
Collaborator Author

lthibault commented Nov 29, 2021

@zenhack Re your question about quotes (can't seem to reply for some odd reason):

  1. The Sprintf call made things readable enough IMO without the strconv.Quote call
  2. Error message is still quoted (quotes are inline, negating the need for strconv.Quote)
  3. An error value can't be passed to strconv.Quote directly. Calling the Error() method seemed verbose.

tl;dr: it was a judgement call about readability. Outputs should be identical, though, and we can revert if you prefer.

@zenhack
Copy link
Contributor

zenhack commented Nov 29, 2021

strconv.Quote is more than just return "\"" + s "\""; it escapes things inside the string as well.

But you can also use a %q format specifier, which would keep the full escaping while also being more concise. It also works with errors.

@lthibault
Copy link
Collaborator Author

strconv.Quote is more than just return "\"" + s "\""; it escapes things inside the string as well.

Oh, I didn't realize that. Makes sense.

But you can also use a %q format specifier, which would keep the full escaping while also being more concise. It also works with errors.

This sounds like our winner. Will push changes shortly.

@zenhack zenhack merged commit 74c8481 into main Nov 30, 2021
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