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

Update unmarshal function to wrap ErrJSONUnmarshal. Requires at least Go 1.13. #924

Merged
merged 1 commit into from May 15, 2021

Conversation

ewohltman
Copy link
Contributor

@ewohltman ewohltman commented May 3, 2021

Update unmarshal function to wrap ErrJSONUnmarshal and return specific error message.

Requires at least Go 1.13.

Resolves #923

Copy link
Contributor

@jolheiser jolheiser left a comment

Choose a reason for hiding this comment

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

LGTM, this seems nice for better troubleshooting and more accurate error responses. 👍

@CarsonHoffman CarsonHoffman merged commit 9c33bfb into bwmarrin:master May 15, 2021
@CarsonHoffman
Copy link
Collaborator

Just for future reference: I considered whether it might make more sense to go all the way and expose the underlying JSON error via wrapping rather than just including its error text, but the following passage from Working with Errors in Go 1.13 is enlightening here:

It’s important to remember that whether you wrap or not, the error text will be the same. A person trying to understand the error will have the same information either way; the choice to wrap is about whether to give programs additional information so they can make more informed decisions, or to withhold that information to preserve an abstraction layer.

It doesn't seem to me like a program using an API wrapper would have any particular course of action based on a transit error, and thus this solution seems reasonable to me. Perhaps someone looking to extract that information in the future will see this—my point of putting my reasoning here—and it might be reasonable to revisit this in the future, but this is a good change now, so thank you for the change!

@FedorLap2006 FedorLap2006 added this to the v0.24.0 milestone Feb 27, 2022
@FedorLap2006 FedorLap2006 added the breaking changes Contains breaking changes. Should be reflected in the changelog label Mar 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking changes Contains breaking changes. Should be reflected in the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Function unmarshal swallows specific error message
4 participants