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

Handle file objects like file_upload #689

Merged
merged 3 commits into from
Sep 24, 2018
Merged

Handle file objects like file_upload #689

merged 3 commits into from
Sep 24, 2018

Conversation

ob-stripe
Copy link
Contributor

@ob-stripe ob-stripe commented Sep 17, 2018

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

  • Renames the Stripe::FileUpload resource class to Stripe::File, and adds a FileUpload alias for the File class
  • Changes the OBJECT_NAME from file_upload to file, and adds an OBJECT_NAME_ALT with value file_upload
  • Updates convert_to_stripe_object to deserialize both file and file_upload objects into Stripe::File objects
  • For tests: I've added a new file_test.rb file but also kept the original file_upload_test.rb file. This is probably unnecessary, and we could remove the latter and simply add a test to check that Stripe::FileUpload == Stripe::File. Let me know what you think is preferable.

extend Stripe::APIOperations::Create
extend Stripe::APIOperations::List

OBJECT_NAME = "file_upload".freeze
OBJECT_NAME = "file".freeze
ALT_OBJECT_NAME = "file_upload".freeze
Copy link
Contributor

Choose a reason for hiding this comment

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

Mind just adding a bit of a comment here explaining that these used to be two distinct objects that were then unified into one?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, sorry for the super nit, but just so this sorts nicely, do you think we could go OBJECT_NAME_ALT? :)

@brandur-stripe
Copy link
Contributor

  • For tests: I've added a new file_test.rb file but also kept the original file_upload_test.rb file. This is probably unnecessary, and we could remove the latter and simply add a test to check that Stripe::FileUpload == Stripe::File. Let me know what you think is preferable.

I think what you've done here is the right move for now.

Left a few minor nits, but LGTM. I think your tests were failing due to an intermittent Travis problem, so I restarted one of the builds in the matrix.

ptal @ob-stripe

@ob-stripe
Copy link
Contributor Author

Fixed, ptal @brandur-stripe

@brandur-stripe
Copy link
Contributor

LGTM.

@ob-stripe
Copy link
Contributor Author

I pushed a second commit to make it so that files.stripe.com is only used for the create request. ptal @brandur-stripe


# Set `api_base` to `nil` to ensure that these requests are _not_ sent
# to the default API hostname.
Stripe.api_base = nil
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a bit awkward, but it's necessary because if api_base and uploads_base have the same value, then assert_requested is not actually asserting that the request is for the correct host.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point. It may be worth adding a comment that notes that api_base will be reset between every test case so this won't mutate the global state of test runs that come after it.

@@ -103,7 +103,7 @@ module Stripe

@api_base = "https://api.stripe.com"
@connect_base = "https://connect.stripe.com"
@uploads_base = "https://uploads.stripe.com"
@uploads_base = "https://files.stripe.com"
Copy link
Contributor

Choose a reason for hiding this comment

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

We may eventually want to rename this to files_base for consistency.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I thought of that but was afraid it should count as a breaking change. wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, true (although hopefully no one's overriding this). It does seem less churnful to just leave it for now though, so NM. If we ever feel strongly about it later, we can always change it then.

@brandur-stripe
Copy link
Contributor

Minor comment above, but LGTM.

@ob-stripe ob-stripe merged commit e20b44d into master Sep 24, 2018
@ob-stripe ob-stripe deleted the ob-file-resource branch September 24, 2018 19:09
@ob-stripe
Copy link
Contributor Author

Released as 3.27.0.

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.

3 participants