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

Check if default is a hash rather than checking if it responds to merge #20840

Conversation

sgringwe
Copy link
Contributor

@sgringwe sgringwe commented Jul 11, 2015

This fixes scenarios where objects (such as nil) have the 'merge' method defined for other reasons. In this situation, the CanCan gem had defined merge: CanCanCommunity/cancancan#229. It is debatable whether cancan or rails is 'at fault' here, but checking if default is a hash feels more correct based on my understanding of the intended behavior.

If you'd like for a test to be added just let me know.

This fixes #20837

…ge. This fixes scenarios where objects (such as nil) have the 'merge' method defined for other reasons.
@Senjai
Copy link
Contributor

@Senjai Senjai commented Jul 12, 2015

@sgringwe See my comment on CanCanCommunity/cancancan#229. I believe duck typing instead of explicitly checking the class is the preferred approach for these scenarios with ruby.

@matthewd
Copy link
Member

@matthewd matthewd commented Jul 12, 2015

I believe duck typing instead of explicitly checking the class is the preferred approach for these scenarios with ruby

Agree

@matthewd matthewd closed this Jul 12, 2015
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 this pull request may close these issues.

3 participants