-
Notifications
You must be signed in to change notification settings - Fork 12
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
Support faraday 2.0 & streaming api #32
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @aka-nez 🙌!
Agree about the CI matrix, please make it cover 2.6 -> 3.1 like we do in Faraday.
Out of curiosity, could you explain why you needed to explicitly add the faraday/multipart
middleware? What kind of errors were you seeing otherwise?
spec/faraday/http/adapter_spec.rb:26 uses this middleware, that was bundled with Faraday before I tried removing this middleware - all specs passed. I will remove this lines with next commit |
Thanks for clarifying! |
I have updated the CI matrix and entirely removed everything related to multipart. It doesn't impact tests |
(I did some cleaning out of earlier Required Build Checks, which included now-removed ones.) |
Ah, I noted this in the failure in CI:
Perhaps some --deployment flag is enabled> Edit:
We can probably drop this, since the bundler-cache make it quick anyway. |
Removed this line from ci |
@iMacTia Are we ready to go, here? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, faraday ~> 2.0
is incorrect, we should pin to faraday ~> 2.5
as older versions don't have the new streaming API. Tests are passing because bundler installs the latest version, which works fine.
Sure. Fixed! 👍 |
Thank you so much @aka-nez, I'm gonna drop a release soon 🙌 ! |
Hi! I tried to update the gem to make it compatible with Faraday 2 and add streaming API support.
Take a look. please @iMacTia
Probably, I should update the CI matrix, too: remove 2.5, add 3.0+