httprb / http

HTTP (The Gem! a.k.a. http.rb) - a fast Ruby HTTP client with a chainable API, streaming support, and timeouts
MIT License
3.01k stars 321 forks source link

Revert `Response#parse` behavior introduced in #540 #670

Closed DannyBen closed 3 years ago

DannyBen commented 3 years ago

Following the discussion in #665, this PR reverts the behavior of Response#parse to how it behaved in 4.x.

paul commented 3 years ago

I also agree with this change. If I'm getting a response back that says its "application/json", but its not, then I have to handle that somewhere. If I do any of JSON(HTTP.get()) or HTTP.get().parse or HTTP.get().parse(:json) doesn't matter, they're all going to fail exactly the same way.

If I'm writing a quick script, then letting it just fail is fine. If I'm doing something more production-worthy, I have to do the manual labor to either rescue the exception, or not use #parse, or just let it go to the exception tracker. All of this is the same if #parse does the easy thing, or forces me to add more boilerplate, so I vote we do the easy thing by default.

ixti commented 3 years ago

If I do any of JSON(HTTP.get()) or HTTP.get().parse or HTTP.get().parse(:json) doesn't matter, they're all going to fail exactly the same way.

My argument is not that JSON parsing will fail, but that if one will add custom MIME type parser, #parse behaviour will become pretty confusing IMO. That was the main reason I had when I proposed to get rid of implicit argument. I guess I always thought of #parse as some sort of simple helper over JSON.parse.

But I have no problems with bringing back implicit mime type guessing.

ixti commented 3 years ago

Thanks for the quick PR. :D

DannyBen commented 3 years ago

I guess I always thought of #parse as some sort of simple helper over JSON.parse.

I completely agree with this. I for one, feel that the argument should be removed. I feel that #parse should do it if it can, and raise an error (a friendly one) if it can't. If someone wants to parse with a specific parser (mime type), then can call it outside the scope of the HTTP gem.

tarcieri commented 3 years ago

@ixti perhaps between this and #664 it'd be good to cut a point release

DannyBen commented 3 years ago

Thanks for the quick PR. :D

Thanks for being flexible and quick to merge.

ixti commented 3 years ago

I guess I always thought of #parse as some sort of simple helper over JSON.parse. I completely agree with this. I for one, feel that the argument should be removed.

That's a good point. I like this idea.

ixti commented 3 years ago

@ixti perhaps between this and #664 it'd be good to cut a point release

Sure. Will tackle that ASAP