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

Use duck typing to detect File-like objects #616

Merged
merged 1 commit into from
Dec 14, 2017

Conversation

ob-stripe
Copy link
Contributor

@ob-stripe ob-stripe commented Dec 14, 2017

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

Use duck typing to detect File-like objects rather than directly checking the class. This is the same strategy used by rest-client: https://github.com/rest-client/rest-client/blob/f450a0f086f1cd1049abbef2a2c66166a1a9ba71/lib/restclient/payload.rb#L55.

Thanks @kimroen for the suggestion!

Fixes #615.

@kimroen
Copy link

kimroen commented Dec 14, 2017

Nice! You don't need to require tempfile anymore now: https://github.com/stripe/stripe-ruby/pull/616/files#diff-df65def818cc84069ef28befc1fb1102L1

@coveralls
Copy link

coveralls commented Dec 14, 2017

Coverage Status

Coverage remained the same at 91.808% when pulling 630c260 on ob-file-duck-typing into be8e4ff on master.

@ob-stripe
Copy link
Contributor Author

Oops! Nice catch @kimroen, thanks!

@coveralls
Copy link

coveralls commented Dec 14, 2017

Coverage Status

Coverage decreased (-0.005%) to 91.803% when pulling d946d7a on ob-file-duck-typing into be8e4ff on master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.005%) to 91.803% when pulling d946d7a on ob-file-duck-typing into be8e4ff on master.

@coveralls
Copy link

coveralls commented Dec 14, 2017

Coverage Status

Coverage decreased (-0.005%) to 91.803% when pulling d946d7a on ob-file-duck-typing into be8e4ff on master.

@brandur-stripe
Copy link
Contributor

This definitely seems better.

@ob-stripe Can you take a look at the Coveralls configuration here? A decrease of 0.005% in coverage probably shouldn't be a CI failure. My preference would be to allow something a lot more generous than that for the time being (1%?) — thoughts?

@ob-stripe
Copy link
Contributor Author

Yep, agreed. I just set the threshold to 1% for all the libraries repos.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.005%) to 91.803% when pulling b83a0e5 on ob-file-duck-typing into be8e4ff on master.

2 similar comments
@coveralls
Copy link

coveralls commented Dec 14, 2017

Coverage Status

Coverage decreased (-0.005%) to 91.803% when pulling b83a0e5 on ob-file-duck-typing into be8e4ff on master.

@coveralls
Copy link

coveralls commented Dec 14, 2017

Coverage Status

Coverage decreased (-0.005%) to 91.803% when pulling b83a0e5 on ob-file-duck-typing into be8e4ff on master.

@ob-stripe
Copy link
Contributor Author

Not sure why coveralls is posting 3 messages instead of 1, but the status check does take take the new threshold into account.

@brandur-stripe
Copy link
Contributor

@ob-stripe Nice! Thanks.

BTW, I think there's a chance that we might be okay just disabling auto-comments completely given that they're a little noisy and that the coverage numbers are displayed down on the Coveralls status line anyway. We can cover that later though.

@brandur-stripe brandur-stripe merged commit 659025c into master Dec 14, 2017
@brandur-stripe brandur-stripe deleted the ob-file-duck-typing branch December 14, 2017 20:11
@ob-stripe
Copy link
Contributor Author

Oh nice, I missed that the actual number was displayed in the status line. I've disabled Coverall comments for now, we can always re-enable them later if we think they're valuable.

@brandur-stripe
Copy link
Contributor

Oh nice, I missed that the actual number was displayed in the status line. I've disabled Coverall comments for now, we can always re-enable them later if we think they're valuable.

+1. TY.

@brandur-stripe
Copy link
Contributor

Released as 3.9.1.

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.

4 participants