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

Revert Response#parse behavior introduced in #540 #670

Merged
merged 1 commit into from
May 26, 2021

Conversation

DannyBen
Copy link
Contributor

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

@DannyBen DannyBen marked this pull request as ready for review May 26, 2021 15:18
@tarcieri tarcieri requested a review from ixti May 26, 2021 15:35
@paul
Copy link
Contributor

paul commented May 26, 2021

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
Copy link
Member

ixti commented May 26, 2021

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 ixti merged commit 40d29cc into httprb:master May 26, 2021
@ixti
Copy link
Member

ixti commented May 26, 2021

Thanks for the quick PR. :D

@DannyBen
Copy link
Contributor Author

DannyBen commented May 26, 2021

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.

@DannyBen DannyBen deleted the revert-parse branch May 26, 2021 17:11
@tarcieri
Copy link
Member

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

@DannyBen
Copy link
Contributor Author

Thanks for the quick PR. :D

Thanks for being flexible and quick to merge.

@ixti
Copy link
Member

ixti commented May 26, 2021

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
Copy link
Member

ixti commented May 26, 2021

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

Sure. Will tackle that ASAP

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