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

Add property accessor for error messages #422

Merged
merged 2 commits into from
May 14, 2018

Conversation

seants
Copy link
Contributor

@seants seants commented Apr 21, 2018

In python3, it's no longer possible to do error.message.

The official python alternative of str(error) does not work with the stripe library because __str__ is overridden and would print

Request ch_ae7361827ab7: Your card was Declined.

which is not user-friendly. The three alternatives I've identified are

error.args[0]
error._message
super(stripe.error.StripeError, error).__str__()

none of which are super elegant. This PR would allow

error.user_message

instead. Happy to rename it to something else too.

@brandur-stripe
Copy link
Contributor

Hey @seants, thanks a lot for the PR!

So I think in the case of Python 3, the preferred method of extracting a message from an error is now just this:

str(error)

Does that look okay as an alternative to you?

@seants
Copy link
Contributor Author

seants commented Apr 24, 2018

Oh sorry I forgot to add that to the description: that doesn’t work for me because that prints

Request ch_1674827andv: your card was declined

And we don’t want to show that first part to the user

@brandur-stripe
Copy link
Contributor

Ah, I see! Thanks for the clarification.

@ob-stripe Do you have an opinion on this one? I could see either doing something similar to the suggested above, and I could also see inverting the problem by removing the "Request xxx: " prefix from the standard string representation and adding a message_with_request method that makes it available.

@seants
Copy link
Contributor Author

seants commented Apr 25, 2018

That would've been nice initially but is a breaking change for all clients that depend on that behavior. I could totally imagine someone doing str(error)[20:] to chop that first part off. This PR is kind of a workaround for that so that it can be released anytime.

@seants
Copy link
Contributor Author

seants commented Apr 25, 2018

In terms of naming, I was gonna go for user_message but I didn't do a deep-dive into whether that message is always user-friendly. As mentioned, happy to rename.

@brandur-stripe
Copy link
Contributor

Hey @seants, sorry about the dropping the ball here! I think I archived this by accident.

Okay, thank you for explanation on backwards compatibility — agreed. Would you mind renaming the method to user_message like you suggested? I like the name better, and I think that regardless of whether it should be user-facing to an end user, it's always going to be a "user message" from the perspective of Stripe (people who have this library installed are our users). Also, would you mind adding a comment above it that just gives a basic explanation of how it compares to the standard string override?

@seants
Copy link
Contributor Author

seants commented May 12, 2018

Haha no worries; yes I'll take care of those this weekend!

@seants
Copy link
Contributor Author

seants commented May 13, 2018

Updated!

@brandur-stripe brandur-stripe merged commit 523e828 into stripe:master May 14, 2018
@brandur-stripe
Copy link
Contributor

Thanks!

Released as 1.82.0.

@seants
Copy link
Contributor Author

seants commented May 14, 2018

Thanks!

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.

None yet

2 participants