mamantoha / crest

HTTP and REST client for Crystal
https://mamantoha.github.io/crest/
MIT License
235 stars 14 forks source link

Possibility to "download by streaming" #101

Closed fusillicode closed 5 years ago

fusillicode commented 6 years ago

Hi, I looked around inside the source but I didn't find an obvious way to achieve the same result of this code:

require "http/client"

HTTP::Client.get("http://example.org") do |response|
  File.write("example.com.html", response.body_io)
end

I think that It may be an interesting feature to develop and one little feature that could make Crest even more awesome.

For any kind of info about the context in which I would personally love to have this feature don't hesitate to ask me, I'll be more than happy to talk about it!

Cheers! 🍻

mamantoha commented 6 years ago

Hi @fusillicode

Thanks for interesting in crest.

In current implementation Crest and Crest::Request verb methods(get, post, etc.) can yields the Crest::Request to the block:

resp = Crest.get("http://httpbin.org/headers") do |request|
  request.headers.add("foo", "bar")
end

According to my observations, no one uses it. I guess this can be changed to executes a request and yields the Crest::Response to the block. E.g.:

resp = Crest.get("http://httpbin.org/headers") do |response|
  File.write("file.html", response.body)
end

But I'm not sure about this.

fusillicode commented 6 years ago

Hi @mamantoha, I'm the one who should be thankful to your for your work and this awesome shard! 😄

Regarding yielding the request I think that it can also be useful in some situations. I'm not sure either if switching to executing the request and yielding the response could be better than yielding the request.

By thinking about it, maybe what I'm suggesting isn't even in the scope of the shard 🤔

Or maybe there is some other way to include such a feature in crest 🤔

P.S: in the project where I'm using crest I'm using the stdlib HTTP::Client to achieve what I need 😉

mamantoha commented 6 years ago

Hi. This make sense.

When I created this library, I had the following thoughts:

request = Crest::Request.new(:get, "http://example.com") do |req|
  req.headers.add("foo", "bar")
 end

response = request.execute
response = Crest::Request.get("http://example.com") do |resp|
  File.write("file.html", resp.body)
end

But eventually, I implemented only first option.

I will try to include this feature in crest :wink: .

fusillicode commented 6 years ago

Awesome!

What you suggest sounds really good to me and it somehow feels as a natural API. I really like it.

I hope to be able to support this feature with a PR.

For anything however do not hesitate to write me!

P.S: you can find me in the crystal lang gitter 😉

mamantoha commented 6 years ago

This is great! Feedback and PR always welcome!

mamantoha commented 5 years ago

https://github.com/mamantoha/crest/pull/106

Fixed in v0.17.0