awesome-print / awesome_print

Pretty print your Ruby objects with style -- in full color and with proper indentation
http://github.com/michaeldv/awesome_print
MIT License
4.07k stars 454 forks source link

Fix for `#to_hash` inspection logic. #285

Closed trevorrowe closed 3 years ago

trevorrowe commented 7 years ago

Objects that respond to #to_hash, but do not define it, i.e. respond via method missing, cause for an unhelpful error to be raised.

Fixes this issue by replacing #respond_to? with checking the object's public methods.

See #275 Fixes #275

gerrywastaken commented 7 years ago

@trevorrowe This is not super significant, but the class never really had a chance of passing as it's checking against a variable called method_name, which is not being passed into respond_to?: https://github.com/trevorrowe/awesome_print/blob/23cf0d1fc89b63e4d262443a5c78ed364e6bd92c/spec/formats_spec.rb#L788

If we swap out the respond_to with a respond_to_missing then method works without the fix in this PR. respond_to_missing is normally called from respond_to automatically and appears to be specifically intended for the case when the method isn't defined but we want to method to work:

it "that responds to #to_hash but doesn't define it" do
  class My
    def method_missing(method_sym, include_private = false)
      method_sym == :to_hash ? {} : super
    end

    def respond_to_missing?(method_sym, include_private = false)
      method_sym == :to_hash || super
    end
  end

  my = My.new
  expect { my.ai(plain: true) }.not_to raise_error
end

Thoughtbot for instance recommends "always define respond_to_missing? when overriding method_missing": https://robots.thoughtbot.com/always-define-respond-to-missing-when-overriding

This is done by stdlib's Delegate automatically so this is not an issue for you.

Following Thoughtbot's advice means that we are trusting the person implementing the class to tell us what they support, instead of trying to inspect their class manually to see what it supports.

This PR hash been very helpful either way as it pointed out to me that the arity check is not quite right as it will never allow the use of your to_hash due to the arity coming back as a -1. This also applies to the your test where it seems like we should be respecting the value returned here:

if method_name == :to_hash
  {}

I believe fixing the arity check may actually fix other issues we are having in some rare cases.

Thank you

BryanH commented 3 years ago

This is very old, so I'm closing it. Feel free to reopen if the above was addressed!