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

Broken with Hashie >= 3.0.0 #26

Closed
KOBA789 opened this issue Apr 3, 2015 · 2 comments · Fixed by #57
Closed

Broken with Hashie >= 3.0.0 #26

KOBA789 opened this issue Apr 3, 2015 · 2 comments · Fixed by #57

Comments

@KOBA789
Copy link

KOBA789 commented Apr 3, 2015

Hashie >= 3.0.0 Mash::respond_to? always returns true when given method name ending with "!", "?", "_", "=".
Ref: hashie/hashie@5ac8516#diff-167a475bc5a73269819928a9da362073R190

It breaks this garage's code:
https://github.com/cookpad/garage/blob/master/lib/garage/representer.rb#L159

Garage assumes instance of Hashie::Mash to be a instance including Garage::Representer.
As a result of it, it fails at here with a message "NoMethodError: undefined method `params' for Hashie::Mash:Class ".

I suggest specifying Hashie < 3.0.0 in gemspec.

@taiki45
Copy link
Contributor

taiki45 commented Apr 3, 2015

I think specifying Hashie < 3.0.0 has bad side, that garage gem restricts users to use Hashie 3.0.0 above.

The one of alternatives is not checking the value responds to represent! but checking the value includes Garage::Representer. Because the value must respond to params= and partial= and more, checking againt represent! is not enought.

I think alternative plan is reasonable. How do you think?

@luvtechno
Copy link
Contributor

We are facing this issue in our garage app, where hashie 3.4.3 is used.

The one of alternatives is not checking the value responds to represent! but checking the value includes Garage::Representer. Because the value must respond to params= and partial= and more, checking againt represent! is not enought.

This sounds good to me -- we are monkey patching as such and going to test with our app.

cc/ @creasty

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 a pull request may close this issue.

3 participants