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

Refactor or remove Response#parse API #538

Closed
ixti opened this issue Mar 5, 2019 · 3 comments
Closed

Refactor or remove Response#parse API #538

ixti opened this issue Mar 5, 2019 · 3 comments

Comments

@ixti
Copy link
Member

ixti commented Mar 5, 2019

IMO Response#parse with implicit MIME-type guessed from response Content-Type must be discouraged:

HTTP.get("http://example.com").parse

The snippet above will raise:

HTTP::Error: Unknown MIME type: text/html

Although error message is clear, I don't see how this API can be good to use. Let's say one will define parser for text/html that will be returning Nokogiri::Document. So now, #parse will be returning Nokogiri::Document if content type is text/html, Array or Hash if content type is application/json... There's no way one will be able to work with that without some sort of wrapper (or just branching based on type) to neglect differences between Nokogiri::Document and plain Array or Hash loaded from JSON.

So probably we should make mime type argument required.

PS Users of Oj may find this API less performant than Oj.load(res) as latter one will utilize streaming.

@ixti
Copy link
Member Author

ixti commented Mar 5, 2019

/cc @httprb/core

@ixti ixti added the Discussion label Mar 5, 2019
@tarcieri
Copy link
Member

tarcieri commented Mar 5, 2019

That sounds ok to me, so long as something like this works:

.parse(:json)

@ixti
Copy link
Member Author

ixti commented Mar 5, 2019

Yeah. That was my point we should remove implicitness of mime type argument.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants