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

Expose response objects #421

Merged
merged 1 commit into from Nov 28, 2017
Merged

Conversation

sedouard
Copy link
Contributor

@sedouard sedouard commented Nov 9, 2017

This PR is addressing #420 to provide access to not only response headers but general data on underlying HTTP requests made by API request to consumers.

Based on some of the discussion on the issue, I'm proposing adding response data to StripeObject and changing the interface of StripeResponse (and adding StripeHeaders class) to be a bit closer to the popular OkHTTP's response class.

@ob-stripe what are your thoughts around this? Once we settle down on what looks best I'll go ahead and include unit tests.

@ob-stripe
Copy link
Contributor

Left a few comments, but lgtm overall! Thanks for tackling this :)

@sedouard sedouard changed the title WIP - Expose response objects Expose response objects Nov 10, 2017
.gitignore Outdated Show resolved Hide resolved
@sedouard
Copy link
Contributor Author

@ob-stripe Should I squash my commits?

@ob-stripe
Copy link
Contributor

This is starting to look great!

@brandur-stripe Mind taking a look too?

@ob-stripe
Copy link
Contributor

@ob-stripe Should I squash my commits?

We don't have a firm policy on this, but that's what I usually do.

@sedouard sedouard force-pushed the success-response-headers branch 3 times, most recently from 6429389 to 715c40b Compare November 14, 2017 18:22
@sedouard
Copy link
Contributor Author

@ob-stripe per stripe/stripe-python#371 I've removed the optional flag for including responses and changed the field for responses to lastResponse.

Copy link
Contributor

@ob-stripe ob-stripe left a comment

Choose a reason for hiding this comment

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

Left a few nitpicky comments, but lgtm overall!

src/main/java/com/stripe/net/StripeResponse.java Outdated Show resolved Hide resolved
src/test/java/com/stripe/net/StripeHeadersTest.java Outdated Show resolved Hide resolved
@sedouard
Copy link
Contributor Author

@ob-stripe thanks! Made all the requested changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants