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

Remove File in favor of FileUpload #585

Merged
merged 1 commit into from
Jun 12, 2018
Merged

Conversation

brandur
Copy link
Contributor

@brandur brandur commented Jun 12, 2018

As we discussed in #578, here we unify the File and FileUpload
models into a single type. We do this by removing File, and changing
all instances where it was referenced to FileUpload.

These two structs were almost identical already, with the exception of
the MIMEType field on File, which is called Type on FileUpload.
Aside from that, the rename should cause very minimal breakage because
most users would have been accessing members without referencing the
struct's name directly like dispute.Evidence.CustomerSignature.URL.

Also, just examining the server-side resources, I'm pretty sure that
MIMEType didn't even work because it looks like the field is actually
called Type for both files and file uploads. So even if a user has
referenced the property, they should fix their use anyway.

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

As we discussed in #578, here we unify the `File` and `FileUpload`
models into a single type. We do this by removing `File`, and changing
all instances where it was referenced to `FileUpload`.

These two structs were almost identical already, with the exception of
the `MIMEType` field on `File`, which is called `Type` on `FileUpload`.
Aside from that, the rename should cause very minimal breakage because
most users would have been accessing members without referencing the
struct's name directly like `dispute.Evidence.CustomerSignature.URL`.

Also, just examining the server-side resources, I'm pretty sure that
`MIMEType` didn't even work because it looks like the field is actually
called `Type` for both files and file uploads. So even if a user has
referenced the property, they should fix their use anyway.
@remi-stripe
Copy link
Contributor

@brandur-stripe Before I dig, do you think we should punt and wait for the new File resource instead that @ob-stripe is working on instead? Since it's shipping soon-ish and would be a good way to unify everything only once.

@brandur-stripe
Copy link
Contributor

@remi-stripe Yeah, good point. I was just re-reading Olivier's documentation on the subject, and while most of it will stay the same, the intent is for file to replace file_upload as the resource name, so I guess long term File will be the better name for this struct.

This isn't a critical refactor or anything, so I'm fine with waiting on that project to finish.

@brandur-stripe
Copy link
Contributor

(Although on the other hand, this is once again a fairly minor breaking change, so it might be plausible just do it now, and then fix the naming again later once that happens. I think both directions are plausible.)

@remi-stripe
Copy link
Contributor

Sounds good. I think we're getting closer so it might be worth putting this one as a future and shipping it in a few weeks.

@ob-stripe if you think it will take longer, let us know and we could ship this for now.

@remi-stripe
Copy link
Contributor

@brandur-stripe Okay that sounds good after all. Let me review the PR!

Copy link
Contributor

@remi-stripe remi-stripe left a comment

Choose a reason for hiding this comment

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

LGTM

@brandur-stripe
Copy link
Contributor

Excellent. Thanks Remi! I'm happy to write up the next patch as Olivier's work continues to progress too.

@brandur-stripe brandur-stripe merged commit 70e9e5c into master Jun 12, 2018
@brandur-stripe brandur-stripe deleted the brandur-remove-file branch June 12, 2018 21:36
@brandur-stripe
Copy link
Contributor

Released as 34.0.0.

nadaismail-stripe pushed a commit that referenced this pull request Oct 18, 2024
* Adding another extension recommendation

* Whitespace fixes

* Typo in data mapper

* Allow refresh tokens to work with default username specified

* SF devtips and refresh-tokens docs

* Adding brennen to PR template

* More doc links

* Update salesforce_devtips.md

Co-authored-by: brennen-stripe <[email protected]>

Co-authored-by: brennen-stripe <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants