Please don't implement .nil?

[patrick-minton]

Ran into this today:

[2] pry(some_code)> response
=> nil
...
[8] pry(some_code)> response == nil
=> false
[9] pry(some_code)> response.nil?
=> true

Had to do this because we were assuming that code like this would cover the nil case:

raise SomeError unless response

Took me a bit of digging to find out that HTTParty::Response implements nil?:

https://github.com/jnunemaker/httparty/blob/master/lib/httparty/response.rb#L58

I don't think this is a good idea. It is widely expected behavior in ruby that nil? isn't overwritten, since nil is a singleton, so these are almost universally treated as equivalent:

some_object == nil
some_object.nil?

This is important because if they're equivalent, you can use "truthiness" to check if the object itself is nil, as opposed to if the response's body is nil, which is a different thing entirely. The definition of nil? above even includes an empty string in the response body, which is, again, not at all the same as when the response itself is nil.

This method really should be .blank? or some such.

[hydrogen18]

This was carried over by mself when I refactored the Response class. I didn't see it as a great idea but was trying not to just yank the rug out from people using this library. I've sat through similar conversations with co-workers as your pry sessions where the code is baffling nil? but also an instance of something other than NilClass

Your points are valid, but we should bump a significant version number( 15->16 here I think) since this is going to break lots of folks code.

[jnunemaker]

I think I'm in favor of this. It very likely could break a lot of people's use but feels like the right thing. I believe I added nil? (if it was me) way back in the day when things were a bit more wild west.

[TheSmartnik]

@jnunemaker In favor as well, but I think we should add a deprecation warning first.
I'll create a pr for this later today

[jaredlt]

A note for future troubleshooters: In my case it wasn't a call to response.nil? directly but rather I was using Nokogiri:

Old code

response = HTTParty.get('https://www.google.com')
page = Nokogiri::HTML(response) # this used to work

Fix

page = Nokogiri::HTML(response.body) # update to this

[viktorradnai]

Another note for future troubleshooters: logging the response object will trigger the depreciation warning as well:

require "httparty"
require 'logger'

logger = Logger.new($stdout)

response = HTTParty.get('https://www.google.com')
logger.debug(response)

[jaredbeck]

In my experience, when a Net::ReadTimeout occurs, then HTTParty.post will return an actual nil. If I'm correct (it's not documented, just my observation) then this is something everyone should check for. Unfortunately, when they do add such a .nil? check, they will now get a deprecation warning.

.. Please, add explicit check response.body.nil? || response.body.empty?. ..

The warning's advice is dangerously misleading. Following the advice would cause a NoMethodError ("no method body for NilClass"). Perhaps the warning's advice can be corrected?

[pkoch]

For anyone looking to suppress these warnings where they can't do anything about them (in my case, in ActiveSupport::Cache), you can install warning and have something like:

Warning.ignore(
    /#{Regexp.quote("HTTParty will no longer override `response#nil?`.")}.+#{Object.const_source_location('ActiveSupport::Cache')[0]}/m,
)

[GeorgeKaraszi]

I would love to see this warning stripped away. Many things, such as loggers and application debuggers, call .nil? under the hood, so this warning gets spammed quite a lot. Most value aggregators rely on that method too since it's Ruby's most fundamental routine for method guarding and is often aliased/used in other things such as Rail's .blank? method.

I.E. it's a warning pretty much completely unavoidable in a lot of circumstances, even if we're not checking or caring about the response body itself.

For those looking to suppress it:

HTTParty::Response.class_eval do
  def warn_about_nil_deprecation
  end
end

[key88sf]

Just curious when in practice the response object is actually nil? It seems like either there is a response (even if it's an http error), or an exception is raised. I've never actually seen the call return and the response object nil. Can you show an example of when this would happen?

[patrick-minton]

It's been a minute since I worked at the company that originally used this code, and which caused me to file this, but I can envision a few ways.

For instance, if the response is fetched in a begin / rescue block, and you did something like this:

response = HTTParty.get('http://no.such.url')

In this case that throws a SocketError. If you catch that error in a rescue block, response is now actually nil

I am sure there are other ways it can happen.

[jaredbeck]

As I mentioned above, a Net::ReadTimeout will also cause post to return an actual nil.

[fmichaut-diff]

Could we get a .blank? method on HTTParty::Response then ?
This would solve the usecase of reponse being nil (since Rail's blank? method works on nil) as well as response.body being blank and all could be tested with response.blank? which would be the previous behaviour of the overiden nil?

Adding a blank? method before the major version change would allow us to migrate the code now without breaking anything and ensure we have removed every warning so we can be ready when the breaking change comes