4.x add deprecation warning to response.parse()

In Version 5 Http.get(url).parse throws ArgumentError "wrong number of arguments (given 0, expected 1)"

Line 159 in 53b7b91

def parse(type)

introduced with #538.

I suggest adding a deprecation warning in Version 4.

That's a good idea.

fa54233

Actually.... I am not sure I understand why this API was changed at all? This change makes the #parse method much less usable. If I know what the content type is, I do not need the HTTP gem to parse it for me - I can JSON.parse it myself.... I was assuming that the intention of the #parse method is to do "best effort" to parse it.

In addition, the response object already knows what the mime type is, so why not use it to parse?

The below code feels wrong:

require 'http'

url = "https://jsonplaceholder.typicode.com/posts/1"
response = HTTP.get url

# Why do we have to do this...
parsed_response = response.parse(response.mime_type)

# ...instead of this?
# parsed_response = response.parse

p parsed_response

To me, the beauty of the HTTP gem is that it "just works".
Can't the #parse method be changed to have its type argument optional?

# lib/http/response.rb
def parse(type = nil)
  type ||= mime_type
  MimeType[type].decode to_s
end

What am I missing?

@DannyBen

@DannyBen

Even if you pass Accept: application/json header, server might respond with plain text, or html, etc. So implicit mime type inferring from the response might lead into a confusing experience IMO:

HTTP.headers(:accept => "application/json").get("https://httpbin.org/html").parse
# HTTP::Error: Unknown MIME type: text/html

Let's say you define HTML decoder for that:

module HTTP
  module MimeType
    class HTML < Adapter
      def decode(str)
        ::Nokogiri::HTML str
      end
    end

    register_adapter "text/html", HTML
    register_alias   "text/html", :html
  end
end

And let's say you expect result to be a JSON, but it comes back as HTML (error or such):

payload = HTTP.headers(:accept => "application/json").get("https://httpbin.org/html").parse

Now you have a parsed response. But it's far from what you expect. So, to be honest I consider parse as a simple helper over JSON.parse(response.to_s), thus I think response.parse(:json) (which can be enhanced to pick up the most efficient adapter) is a better choice.

See Also: #538

It's easy enough to revert this API change locally:

module HTTP
  class Response
    module ImplicitParse
      def parse(as = mime_type)
        super(as)
      end
    end

    prepend ImplicitParse
  end
end

You can also overload HTTP::Response#parse completely.

And let's say you expect result to be a JSON, but it comes back as HTML (error or such):

We can say a lot of things.... 😜

If the response comes back with unexpected content, anything will fail. I don't see the point of this line of questioning. This will also fail with me calling response.parse(:json).

I can understand a case where the Response object does not handle any parsing.
I can understand a case where it uses its own mime type to provide a parsed response "out of the box".
I cannot understand the purpose of a parse "wrapper" that accepts an argument and just calls an adapter. This does not seem like related to the scope of what an HTTP client should do, unless it is being "smart" about it and infers the response type automatically.

Anyways, that is my opinion. Not a big deal from my own standpoint (I switched to JSON.parse), but I feel it is the wrong direction for this particular feature.

Since we are already off topic, i can contribute my 2 cents.

How about if we let parse() be automatic again, but require an allow list to be provided.
So only if the mimetype matches the expected one, automatic parsing takes place.

Http.get(url).parse(:json, :xml)

if response is json or xml it's parsed
if response type anything else it throws an error

Since we are already off topic

Sorry, this is probably my bad. I thought we are on topic, since opening another topic on the subject would have conflicted with this one. My on-topic thought was "don't deprecate, revert".

Http.get(url).parse(:json, :xml)

Personally, I do not see the point in that. It is not the responsibility of an HTTP library to provide parsing wrappers. It is the responsibility of a non-bare-bones HTTP library (such as the HTTP gem in my understanding), to provide automatic parsing when it can do so easily and reliably.

I do not know what was the past implementation that prompted the authors to change it, but in my view "automatic" does not mean "make a guess". It simply means "use the information available to you". So, if the response object has a mime type, and if it knows how to parse this mime type, then Response#parse can be used to automatically get the parsed output. Otherwise, calling #parse should result in some friendly UnspecifiedMimeType exception or similar.

I do not think that situations where the server does not return a mime type, or when it returns different mime types for the same endpoint should be the used as a guide for the implementation. These are edge cases of badly behaving servers. If some users face this, they should test for the mime type themselves and act accordingly.

Lastly, I do not see an advantage of calling response.parse :json instead of JSON.parse response. This is just masking a native Ruby behavior behind a method that needs maintaining and explaining.

IMHO on-topic would have been there: #538

IMHO on-topic would have been there: #538

A closed 2 years old topic? I doubt any reply there would have received any attention, but one thing is certain - discussing whether or not this is on topic, is by itself off topic... :)

Bottom line, your call of course how to handle this. If there is any weight to my opinion, I say either make it parse by mime type (and fail gracefully if it can't), or remove the parse method altogether. Any other route is awkward at best, and bad form at worst.

If you really liked parse() with no argument and want it back, I'd suggest opening a PR that reverts #538 and we can discuss it there.

I've reverted the deprecation warning in 4.x.x branch, further discussion of #parse future should go in #670

Navigation Menu

Repository navigation

4.x add deprecation warning to response.parse() #665

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

4.x add deprecation warning to response.parse() #665

Description

Activity

tarcieri commented on May 18, 2021

ixti commented on May 19, 2021

DannyBen commented on May 25, 2021

ixti commented on May 25, 2021

ixti commented on May 25, 2021

DannyBen commented on May 25, 2021

ronald commented on May 26, 2021

DannyBen commented on May 26, 2021

ronald commented on May 26, 2021

DannyBen commented on May 26, 2021

tarcieri commented on May 26, 2021

ixti commented on May 26, 2021

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions