response no longer string-compatible


Adam Wiggins <adam@...>
 

I see that as of 1.4, the response is no longer a string. As near as
I can tell, this breaks almost every existing use of RestClient. e.g.
the very common codepath like this:

hash = JSON.parse(RestClient.get(url))

Sorry if I haven't been paying attention on the list, but what
motivated this change? Returning a string that can be immediately
parsed is what I think of as the most fundamental feature of
RestClient.

Adam


"François Beausoleil <francois@...>
 

Does the response implement #to_str? This way, the response can be converted to a String very easily.

According to Ruby's documentation, #to_str allows the receiver to be treated as a String where it makes sense.

Bye!
François


Blake Mizerany <blake.mizerany@...>
 

Francois,

RestClient.get("http://foobar.heroku.com/") == "Hello World!" # => false

2010/3/18 François Beausoleil <francois@...>:

Does the response implement #to_str? This way, the response can be converted to a String very easily.

According to Ruby's documentation, #to_str allows the receiver to be treated as a String where it makes sense.

Bye!
François
--
Blake Mizerany
blake.mizerany@...


code@...
 

I see that as of 1.4, the response is no longer a string. As near as
I can tell, this breaks almost every existing use of RestClient. e.g.
the very common codepath like this:

hash = JSON.parse(RestClient.get(url))

Sorry if I haven't been paying attention on the list, but what
motivated this change? Returning a string that can be immediately
parsed is what I think of as the most fundamental feature of
RestClient.

Adam
I see that as of 1.4, the response is no longer a string. As near as
I can tell, this breaks almost every existing use of RestClient. e.g.
the very common codepath like this:

hash = JSON.parse(RestClient.get(url))

Sorry if I haven't been paying attention on the list, but what
motivated this change? Returning a string that can be immediately
parsed is what I think of as the most fundamental feature of
RestClient.

Adam

The motivation was a proposition from you that started in a private
exchange that I then transmaitted on the mailing list (mail 29.01.2010
21:30) :

###

RestClient::Reponse is a little wonky right now - there's this weird
mixin thing, in general it needs some cleanup and normalization.
could you give me some hints of what parts need cleanup or
normalization ?
The thing that seems weird to me is that it's a mixin. You've got
RawResponse and Response, which both include
RestClient::Mixin::Response. It seems like this should be unified
somehow. If nothing else, having "Mixin" in a class name (and a weird
subdirectory named mixin with exactly one file) seems very odd to me.
All of this code was contributed by others so I'm not exactly sure
what the thinking was here - I suspect they were trying to preserve
existing behavior, but the result is wonky code.

Also, the fact that Response is a subclass of String isn't great
practice. Again, this reflects this history - the response was a
string for the first many verisons of restclient. But perhaps we can
start to move away from that now - use duck typing to make sure it
behaves like a string, rather than having it be inherited.

If you want to discuss further let's move to the mailing list. I
think it's really important that everyone be able to see these
discussions.
As the 1.3.0 is released this seems a good way to start the 1.4.0, so
I commited a proposed solution on the master branch.

###

Sorry if I misunderstood what you meant

The updated required some changes in RestClient usages (I provided fix to
couchrest and heroku by myself) but at the end people seemed ok so I think
your point seemed valid.



@François : Response currently implements #to_s but adding a #to_str seems
a good idea, in the same way should I add a #to_i that returns the code ?

@Blake : it's strange as
http://github.com/archiloque/rest-client/blob/1.4.2/lib/restclient/response.rb#L25
should make the 'RestClient.get("http://foobar.heroku.com/") == "Hello
World!"' works (if reponse == foo fail the code then try response.body
with a warning if it matches)



Bien à vous

A.


Archiloque <code@...>
 

Hi,

I've sorted it out with Adams on the irc, Adams:
- perhaps that's where the misunderstanding was - what I was saying is that moving away from that approach internally would be good, *if* we can keep the external interface the same
- perhaps it's just not possible, in which case I think the string inheritence is ok
- as far as I know it's never caused any real problems, it's just wonky when you read the code
- here's the case that absolutely positively must work, regardless of anything else:

###
require 'restclient'
require 'json'

hash = JSON.parse RestClient.get('http://json-test.heroku.com/', :accept => :json)

if hash['hello'] != 'world'
puts "Failed"
exit 1
end

puts "Success"
###

With the 1.4.2 code it fails
/Library/Ruby/Gems/1.8/gems/json_pure-1.2.0/lib/json/pure/parser.rb:103:in `initialize': can't convert RestClient::Response into String (TypeError)

And after following François suggestion and adding Response.to_str (and Response.to_i), it works with some warnings ("The Response is no more a String and the Response content is now accessed through Response.body, please update your code") because json call the #[] method on the Response to do the parsing.

The current behavior is:
- #to_i returns the reponse code
- #body, #to_s and #to_str return the response body
- calling a method missing on Response but available on Response.body returns the expected result but prints a warning

So the critical issue is fixed, but we need your opinion:
- I think the current behavior is a good thing because the semantic of Response.body is better and makes the code cleaner.
- Adam thinks that using Response directly is really handy, and would prefer to remove the warning

What do you think ?


Regards

A.


"François Beausoleil <francois@...>
 

I'd remove the warning. We're pretty much committed to this API for a while. If this is to be revisited, it would be a 2.0 change.

Bye,
François


Blake Mizerany <blake.mizerany@...>
 

I'm not against returning a String. I was against returning a class
that inherited from a string.

Strings are so flexible because they are Strings and Ruby has a
special place in it's heart for them. They are almost 100%
first-class citizens. When you inherit from a string, only to add a
few simple attributes, you remove this relationship.

I propose a simple mixin:

module RestClient
module Response
attr_accessor :code, ....

def body ; self ; end
end

def get(...)
response = make_request(...) # returns a String
response.extend Response
end
end

This keeps it a String while gaining the additional desired behavior.

That is all. I'm not using RestClient as much as I used to so take
this with a grain of salt.

BTW: A module is a mixin. No need to namespace with Mixin IMHO. ;)

-blake

On Fri, Mar 19, 2010 at 12:31 AM, <code@...> wrote:
I see that as of 1.4, the response is no longer a string.  As near as
I can tell, this breaks almost every existing use of RestClient.  e.g.
the very common codepath like this:

hash = JSON.parse(RestClient.get(url))

Sorry if I haven't been paying attention on the list, but what
motivated this change?  Returning a string that can be immediately
parsed is what I think of as the most fundamental feature of
RestClient.

Adam
I see that as of 1.4, the response is no longer a string.  As near as
I can tell, this breaks almost every existing use of RestClient.  e.g.
the very common codepath like this:

hash = JSON.parse(RestClient.get(url))

Sorry if I haven't been paying attention on the list, but what
motivated this change?  Returning a string that can be immediately
parsed is what I think of as the most fundamental feature of
RestClient.

Adam

The motivation was a proposition from you that started in a private
exchange that I then transmaitted on the mailing list (mail 29.01.2010
21:30) :

###

RestClient::Reponse is a little wonky right now - there's this weird
mixin thing, in general it needs some cleanup and normalization.
could you give me some hints of what parts need cleanup or
normalization ?
The thing that seems weird to me is that it's a mixin.  You've got
RawResponse and Response, which both include
RestClient::Mixin::Response.  It seems like this should be unified
somehow.  If nothing else, having "Mixin" in a class name (and a weird
subdirectory named mixin with exactly one file) seems very odd to me.
All of this code was contributed by others so I'm not exactly sure
what the thinking was here - I suspect they were trying to preserve
existing behavior, but the result is wonky code.

Also, the fact that Response is a subclass of String isn't great
practice.  Again, this reflects this history - the response was a
string for the first many verisons of restclient.  But perhaps we can
start to move away from that now - use duck typing to make sure it
behaves like a string, rather than having it be inherited.

If you want to discuss further let's move to the mailing list.  I
think it's really important that everyone be able to see these
discussions.
As the 1.3.0 is released this seems a good way to start the 1.4.0, so
I commited a proposed solution on the master branch.

###

Sorry if I misunderstood what you meant

The updated required some changes in RestClient usages (I provided fix to
couchrest and heroku by myself) but at the end people seemed ok so I think
your point seemed valid.



@François : Response currently implements #to_s but adding a #to_str seems
a good idea,  in the same way should I add a #to_i that returns the code ?

@Blake : it's strange as
http://github.com/archiloque/rest-client/blob/1.4.2/lib/restclient/response.rb#L25
should make the 'RestClient.get("http://foobar.heroku.com/") == "Hello
World!"' works (if reponse == foo fail the code then try response.body
with a warning if it matches)



Bien à vous

A.


--
Blake Mizerany
blake.mizerany@...


Archiloque <code@...>
 

I think it's a really good idea: compared to the inherited propostions on one way it's even more hackish but on another way it's more consistent

so +1 for Blake's idea

Adam, what's your opinion ?

and the other who perhaps have their own usages ?

A.

Le 19 mars 2010 à 21:53, Blake Mizerany a écrit :

I'm not against returning a String. I was against returning a class
that inherited from a string.

Strings are so flexible because they are Strings and Ruby has a
special place in it's heart for them. They are almost 100%
first-class citizens. When you inherit from a string, only to add a
few simple attributes, you remove this relationship.

I propose a simple mixin:

module RestClient
module Response
attr_accessor :code, ....

def body ; self ; end
end

def get(...)
response = make_request(...) # returns a String
response.extend Response
end
end

This keeps it a String while gaining the additional desired behavior.

That is all. I'm not using RestClient as much as I used to so take
this with a grain of salt.

BTW: A module is a mixin. No need to namespace with Mixin IMHO. ;)

-blake

On Fri, Mar 19, 2010 at 12:31 AM, <code@...> wrote:
I see that as of 1.4, the response is no longer a string. As near as
I can tell, this breaks almost every existing use of RestClient. e.g.
the very common codepath like this:

hash = JSON.parse(RestClient.get(url))

Sorry if I haven't been paying attention on the list, but what
motivated this change? Returning a string that can be immediately
parsed is what I think of as the most fundamental feature of
RestClient.

Adam
I see that as of 1.4, the response is no longer a string. As near as
I can tell, this breaks almost every existing use of RestClient. e.g.
the very common codepath like this:

hash = JSON.parse(RestClient.get(url))

Sorry if I haven't been paying attention on the list, but what
motivated this change? Returning a string that can be immediately
parsed is what I think of as the most fundamental feature of
RestClient.

Adam

The motivation was a proposition from you that started in a private
exchange that I then transmaitted on the mailing list (mail 29.01.2010
21:30) :

###

RestClient::Reponse is a little wonky right now - there's this weird
mixin thing, in general it needs some cleanup and normalization.
could you give me some hints of what parts need cleanup or
normalization ?
The thing that seems weird to me is that it's a mixin. You've got
RawResponse and Response, which both include
RestClient::Mixin::Response. It seems like this should be unified
somehow. If nothing else, having "Mixin" in a class name (and a weird
subdirectory named mixin with exactly one file) seems very odd to me.
All of this code was contributed by others so I'm not exactly sure
what the thinking was here - I suspect they were trying to preserve
existing behavior, but the result is wonky code.

Also, the fact that Response is a subclass of String isn't great
practice. Again, this reflects this history - the response was a
string for the first many verisons of restclient. But perhaps we can
start to move away from that now - use duck typing to make sure it
behaves like a string, rather than having it be inherited.

If you want to discuss further let's move to the mailing list. I
think it's really important that everyone be able to see these
discussions.
As the 1.3.0 is released this seems a good way to start the 1.4.0, so
I commited a proposed solution on the master branch.

###

Sorry if I misunderstood what you meant

The updated required some changes in RestClient usages (I provided fix to
couchrest and heroku by myself) but at the end people seemed ok so I think
your point seemed valid.



@François : Response currently implements #to_s but adding a #to_str seems
a good idea, in the same way should I add a #to_i that returns the code ?

@Blake : it's strange as
http://github.com/archiloque/rest-client/blob/1.4.2/lib/restclient/response.rb#L25
should make the 'RestClient.get("http://foobar.heroku.com/") == "Hello
World!"' works (if reponse == foo fail the code then try response.body
with a warning if it matches)



Bien à vous

A.




--
Blake Mizerany
blake.mizerany@...


Cyril Rohr <cyril.rohr@...>
 

Hi,

Just to give my opinion:
I actually liked the change to Response#body even though it made me refactor quite a bit of code. I find it cleaner and more expressive, so I'd be in favor of keeping the warning and a change in the API for a later version.
For me it's important to finally clearly differentiate and make available methods for the body, status, and headers and state that a response is not (just) a body.

But considering the (probably) large number of legacy codes in circulation that use the response as a string, and the responses to this thread, I guess we'll need to support both styles ;-)

Cyril
--
http://crohr.me

On Mar 19, 2010, at 9:10 PM, Archiloque wrote:

Hi,

I've sorted it out with Adams on the irc, Adams:
- perhaps that's where the misunderstanding was - what I was saying is that moving away from that approach internally would be good, *if* we can keep the external interface the same
- perhaps it's just not possible, in which case I think the string inheritence is ok
- as far as I know it's never caused any real problems, it's just wonky when you read the code
- here's the case that absolutely positively must work, regardless of anything else:

###
require 'restclient'
require 'json'

hash = JSON.parse RestClient.get('http://json-test.heroku.com/', :accept => :json)

if hash['hello'] != 'world'
puts "Failed"
exit 1
end

puts "Success"
###

With the 1.4.2 code it fails
/Library/Ruby/Gems/1.8/gems/json_pure-1.2.0/lib/json/pure/parser.rb:103:in `initialize': can't convert RestClient::Response into String (TypeError)

And after following François suggestion and adding Response.to_str (and Response.to_i), it works with some warnings ("The Response is no more a String and the Response content is now accessed through Response.body, please update your code") because json call the #[] method on the Response to do the parsing.

The current behavior is:
- #to_i returns the reponse code
- #body, #to_s and #to_str return the response body
- calling a method missing on Response but available on Response.body returns the expected result but prints a warning

So the critical issue is fixed, but we need your opinion:
- I think the current behavior is a good thing because the semantic of Response.body is better and makes the code cleaner.
- Adam thinks that using Response directly is really handy, and would prefer to remove the warning

What do you think ?


Regards

A.


Adam Wiggins <adam@...>
 

I'm neutral on the implementation (String inheritance, to_str, or
extending the instantiated string). As long as I get access to the
string without .body, I'm happy.

Also, +1 on Blake's point about not using the word "mixin" in the name
of a class, directory, or file.

Adam


code@archiloque.net <code@...>
 

On Fri, 26 Mar 2010 19:25:45 -0700 Adam Wiggins <adam@...> wrote

I'm neutral on the implementation (String inheritance, to_str, or
extending the instantiated string). As long as I get access to the
string without .body, I'm happy.

Also, +1 on Blake's point about not using the word "mixin" in the name
of a class, directory, or file.

Adam
So we'll go with Blake's option and keep the #body method for those to keep the
code cleaner like me and Cyril

Dos anybody have a gripe with this solution ?

A.


Nicholas Wieland <ngw@...>
 

On Mar 27, 2010, at 3:30 AM, code@... wrote:

On Fri, 26 Mar 2010 19:25:45 -0700 Adam Wiggins <adam@...> wrote

I'm neutral on the implementation (String inheritance, to_str, or
extending the instantiated string).  As long as I get access to the
string without .body, I'm happy.

Also, +1 on Blake's point about not using the word "mixin" in the name
of a class, directory, or file.

So we'll go with Blake's option and keep the #body method for those to keep the
code cleaner like me and Cyril

Dos anybody have a gripe with this solution ?

Perfect for me.

  ngw


Archiloque <code@...>
 

Le 27 mars 2010 à 11:30, code@... a écrit :

On Fri, 26 Mar 2010 19:25:45 -0700 Adam Wiggins <adam@...> wrote

I'm neutral on the implementation (String inheritance, to_str, or
extending the instantiated string). As long as I get access to the
string without .body, I'm happy.

Also, +1 on Blake's point about not using the word "mixin" in the name
of a class, directory, or file.

Adam
So we'll go with Blake's option and keep the #body method for those to keep the
code cleaner like me and Cyril

Dos anybody have a gripe with this solution ?

A.

Here we are
http://github.com/archiloque/rest-client/tree/response-is-a-string

now waiting for acks :-)

A.