Skip to content

Refactor form encoding#857

Merged
ob-stripe merged 1 commit intointegration-client-refactorfrom
ob-form-encoder
Oct 24, 2019
Merged

Refactor form encoding#857
ob-stripe merged 1 commit intointegration-client-refactorfrom
ob-form-encoder

Conversation

@ob-stripe
Copy link
Contributor

r? @brandur-stripe @remi-stripe
cc @stripe/api-libraries

Refactors how the library handles form encoding, by moving all form encoding related methods out of LiveStripeResponseGetter and into a new utility class FormEncoder.

This is similar to the architecture we use in stripe-dotnet.

The code itself is also very similar to stripe-dotnet, but stripe-dotnet's code was also based on stripe-java, so all in all it's not too different. The new code is a bit more clean and supports encoding a few more types that we didn't before (Date, enums, arrays of primitives).

None of the methods i removed from LiveStripeResponseGetter were public, so this is not a breaking change.

@ob-stripe
Copy link
Contributor Author

r?+ @richardm-stripe

@richardm-stripe
Copy link
Contributor

richardm-stripe commented Oct 21, 2019

With the "I am not a Java programmer" caveat this seems sensible enough to me and the test suite seems thorough. Obviously passing something cyclical will make this blow up.
It's regrettable that "Object" is passed around so much. but I don't really see a way to avoid!

@ob-stripe
Copy link
Contributor Author

Obviously passing something cyclical will make this blow up.

As obvious as it is, I hadn't considered it before 😂 I think that would qualify as a user error and we don't need to go out of our way to catch it, but at the same time it might not be too hard to track the references we've already seen while flatting the parameters to make sure we're not processing the same reference twice. I'll give it a try.

@ob-stripe ob-stripe changed the base branch from master to integration-client-refactor October 24, 2019 22:29
@ob-stripe
Copy link
Contributor Author

Merging this into a temporary integration branch for now.

@ob-stripe ob-stripe merged commit 740270f into integration-client-refactor Oct 24, 2019
@ob-stripe ob-stripe deleted the ob-form-encoder branch October 24, 2019 22:29
@ob-stripe ob-stripe mentioned this pull request Oct 24, 2019
22 tasks
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.

5 participants