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

Add Response#parse #61

Closed
wants to merge 2 commits into from
Closed

Add Response#parse #61

wants to merge 2 commits into from

Conversation

ixti
Copy link
Member

@ixti ixti commented Jan 19, 2014

Access to parsed body was nuked by merging #39.
This patch brings it back (feature, not the API).

Example
require 'json'

res = HTTP.get('https://api.github.com/repos/tarcieri/http')
res.parse['description']
# => "The HTTP Gem: a simple Ruby DSL for making HTTP requests"

@@ -118,6 +118,18 @@ def to_s
body.to_s
end

# Return parsed response body according to its content type
def parsed
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally I'd prefer calling this method just #parse. Would be curious what others think.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dislike parse as it's a verb... I might be wrong here.
Probably?:

def parse
  # ...
end
alias :parsed :parse

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After discussion I agreed that #parse is better, as it's better conforms with #to_s.

@tarcieri
Copy link
Member

Can you rebase against master?

@sferik, thoughts on this?

@sferik
Copy link
Contributor

sferik commented Jan 20, 2014

To be honest, the fact that this library attempts to handle MIME types seems a bit ambitious. I suppose this is a nice convenience, as long as we only support JSON. Not sure about the name parse. To me, it would be a bit more natural to say response.to_json (or to_whatever for whatever MIME type). Of course, this would require using method_missing. Maybe we should spend a bit more time bikeshedding the method name before merging?

Also it seems strange that the consumer needs to require 'json'. Shouldn’t that be abstracted by the parse method (or whatever we call it)?

@tarcieri
Copy link
Member

To be honest, the fact that this library attempts to handle MIME types seems a bit ambitious.

I could go either way on this. I think it's convenient, but if it doesn't do a good job it probably isn't helping anyone.

I suppose this is a nice convenience, as long as we only support JSON

Seems good

Maybe we should spend a bit more time bikeshedding the method name before merging?

I am also down to discuss this as I too am not super happy with #parse.

Also it seems strange that the consumer needs to require 'json'. Shouldn’t that be abstracted by the parse method (or whatever we call it)?

Agree. require 'json' is silly since it's available everywhere except 1.8.

Access to parsed body was nuked by merging #39.
This patch brings it back (feature, not the API).

Example:

    require 'json'

    res = HTTP.get('https://api.github.com/repos/tarcieri/http')
    res.parse['description']
    # => "The HTTP Gem: a simple Ruby DSL for making HTTP requests"
@ixti
Copy link
Member Author

ixti commented Jan 21, 2014

@tarcieri rebased
@sferik I dislike idea of to_json as it's absolutely irrelevant. In a worst case it's from_json.

I agree that require 'json' is silly. But it seems to me not very good to use default json as well.
So here's my thoughts:

  1. We should use MultiJSON for JSON parser
  2. We should also support XML using MultiXML out of box
  3. Neither MultiJSON nor MultiJSON should become dependencies, instead an appropriate error should be used if one used #parse method and no such library loaded:
json.prepare do
  begin
    require "multi_json"
  rescue LoadError
    fail "MultiJSON was not required"
  end
end
  1. MimeType API should be reworked:
    4.a) it should be a simple inheritance (see below)
    4.b) it should allow association with multiple mime types
# MimeType API changes proposal
module HTTP
  # Yes, HTTP bundles its own MIME type library. Maybe it should be spun off
  # as a separate gem or something.
  class MimeType
    def ready?
      prepare unless @ready
      @ready = true
    end

    # should be implemented on concrete adapter
    def parse(obj)
      obj
    end

    # should be implemented on concrete adapter
    def emit(obj)
      obj
    end

    # should be implemented on concrete adapter
    def prepare; end

    def depend_on(lib)
      require lib
    rescue LoadError
      fail "#{self.class.adapter_name} adapter depends on #{lib}"
    end
    private :depend_on

    class << self
      def adapter_name shortcut
        # ...
      end

      def adapter_mime *mimetypes
        # ...
      end
    end
end

# built in adapters
require 'http/mime_types/json'
require 'http/mime_types/xml'

The proposed idea will allow following adapter declaration:

class HTTP::MimeType::JSON < HTTP::MimeType
  adapter_name :json
  adapter_mime "application/json"

  def prepare
    depend_on "multi_json"
  end

  def parse(obj)
    MultiJSON.load obj
  end

  def emit(obj)
    MultiJSON.dump obj
  end
end

@ixti
Copy link
Member Author

ixti commented Jan 21, 2014

My main reason why I need multiply MIME-types per adapter is because I need to parse application/atom+xml as XML the api above would allow me to:

# append another mime type to be served by XML adapter
HTTP::MimeType::XML.addapter_mime "application/atom+xml"

@tarcieri
Copy link
Member

I'm not a fan of MultiJSON:

  1. It's an onerous dependency
  2. It's an unnecessary dependency in a post 1.9 world where every Ruby implementation can ship a native JSON parser

For the most part I like your MimeType API proposal

@ixti
Copy link
Member Author

ixti commented Jan 21, 2014

OK. I tend to agree about MultiJSON. In a worst case if one would like to use OJ he will be able to easily monkey-patch built-in adapter after all. In this case it's really worth to support only JSON out of the box. Will serve two purposes: fairly common adapter (for Web) and example of how to write adapters.

I think I'll raise different PR with API changes for MimeType - it won't affect this PR anyway.

@ixti
Copy link
Member Author

ixti commented Jan 22, 2014

After some discussions I'm closing this PR. MimeType was broken and it's current state doesn't worth to spend time on updating it.

@ixti ixti closed this Jan 22, 2014
@ixti ixti deleted the fix/parse-by-mime branch January 22, 2014 02:13
@ixti
Copy link
Member Author

ixti commented Jan 22, 2014

Closed in favor of #64 :D

@sferik
Copy link
Contributor

sferik commented Jan 24, 2014

I dislike idea of to_json as it's absolutely irrelevant. In a worst case it's from_json.

I mean to_hash (or to_h).

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.

3 participants