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

Don't wrap request bodies if they are already Readables #38

Merged
merged 4 commits into from
Jul 16, 2024

Conversation

jakeonfire
Copy link
Contributor

@jakeonfire jakeonfire commented Jul 13, 2024

We don't need to wrap request bodies if they already conform to the ::Protocol::HTTP::Body::Readable interface. This way developers can use other kinds of Readables directly when making calls with Faraday.

Use-case: We need to make several requests to an API with chunks of a file in specific byte ranges. Protocol::HTTP::Body::File (from https://github.com/socketry/protocol-http) seems like a good candidate for this:

file_io = URI(file_url).open
byte_ranges.each do |first_byte, last_byte|
  file_io.open if file_io.closed? # it's closed at the end of each post
  body = Protocol::HTTP::Body::File.new(file_io, first_byte..last_byte)
  Faraday.post("https://example.com/uploads", body)
end

Types of Changes

  • New feature.

Contribution

We don't need to wrap request bodies if they already conform to the `::Protocol::HTTP::Body::Readable` interface. This way developers can use other kinds of `Readable`s directly when making calls with `Faraday`.
@jakeonfire
Copy link
Contributor Author

i can write a spec if you think it'd be useful

@ioquatix
Copy link
Member

This looks like a reasonable change, but yes, great to include a test to prevent regressions.

@jakeonfire
Copy link
Contributor Author

test added 👍 are the truffleruby and jruby failures expected? they don't look related...

@ioquatix ioquatix merged commit 7ffe926 into socketry:main Jul 16, 2024
16 of 19 checks passed
@jakeonfire jakeonfire deleted the patch-1 branch July 16, 2024 20:26
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.

2 participants