mwunsch / weary

A framework and DSL for building RESTful web service clients
MIT License
482 stars 23 forks source link

Added XML Parsing and Loosened MultiJson dependency #20

Closed aaronchi closed 11 years ago

mwunsch commented 11 years ago

Hmm, don't feel like this is as worthwhile as json parsing built-in (since we also do json serialization on the Request), but if you think this is a big deal, can you provide a spec?

Why loosen the multi_json requirement? Are you seeing conflicts?

aaronchi commented 11 years ago

I can put together a spec. I just ran into a case where the api only output XML. For multi_json, yes it was conflicting with other gems since it is not the most recent version. Most other gems I've seen don't add a version requirement which just takes the latest.

mwunsch commented 11 years ago

I'm going to reject this request, but I think I've provided some additional functionality that will help you with this use case.

I think Weary ought to be very opinionated about the types of Services it is optimized for, and it's starting to be much more rare to see XML in responses. I think that allowing Response#parse to become a switch statement for different formats is a bad idea, since what you're expecting from the response should be really explicit, and anything unexpected should raise a flag. I also don't want to increase Weary's dependencies.

However, I still see value in handing off response parsing to Weary. So in commit 129377e, I added the ability to add a block to Response#parse that accepts the body and content_type, so in your calling code you can do:

my_response.parse do |body, content_type|
  if content_type.match(/(xml)($|;.*)/)[1]
    MultiXml.parse body  
  end
end

This allows the client to do whatever content negotiation it needs to get something meaningful from the response.

I also think it's dangerous to loosen the requirements Weary's dependencies. Stuff will break and it will be unclear to the implementor why it broke.

Thanks for the pull request, and I hope this solution is satisfactory.