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 response unmarshalling #725

Merged
merged 2 commits into from
Nov 21, 2018

Conversation

EVODelavega
Copy link
Contributor

Looking through some of the code, I noticed errors were manually unmarshalled using a ton of type assertions and maps. That's not good. The problem was that the error types don't export their underlying stripeErr fields. json.Unmarshal uses reflection and tags to find fields that are exported (otherwise it can't set values). I've created an un-exported rawErr type that can be used for unmarshalling, and a further type internalError that embeds the Error type as it is, and adds the decline_code field at the same level (to correspond to the error format).

Ideally, the decline_code field should be a part of the main Error type, so we don't need that second internal type. The type of decline_code was (and is) string. I've left it as is to not break any existing code, but if a field is tagged as omitempty, a pointer type should be used.

Tests all pass, no behavioural changes. The code is just a bit less verbose, more readable, easier to maintain, less error-prone, and possibly faster (though I didn't write a Bench test).

Copy link
Contributor

@brandur-stripe brandur-stripe left a comment

Choose a reason for hiding this comment

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

Thanks @EVODelavega! Looks like an improvement to me.

I added some review comments below, but the general approach looks fine.

error.go Outdated Show resolved Hide resolved
error.go Outdated Show resolved Hide resolved
error.go Outdated Show resolved Hide resolved
error.go Outdated Show resolved Hide resolved
error.go Outdated Show resolved Hide resolved
stripe.go Outdated Show resolved Hide resolved
@brandur-stripe
Copy link
Contributor

LGTM. Thanks!

@brandur-stripe brandur-stripe merged commit 7cad92a into stripe:master Nov 21, 2018
@brandur-stripe
Copy link
Contributor

Released as 53.2.0.

nadaismail-stripe pushed a commit that referenced this pull request Oct 18, 2024
* Remove sorbet runtime var

it's not working

* Additional logging statements
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