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

Add Hash#includes?(other : Hash) overload #7499

Closed
wants to merge 1 commit into from

Conversation

Sija
Copy link
Contributor

@Sija Sija commented Mar 3, 2019

This PR allows checking if a Hash includes given subset denoted by given other Hash.
Functionality asked for instance at SO - Ruby version.

Supersedes #7498 since it's a more versatile implementation, enhancing Hash API without a need for introducing special cases in ContainExpection matcher.

@j8r
Copy link
Contributor

j8r commented Mar 3, 2019

This doesn't closes #7497 ...
I'm not for or against this feature, at least I'm glad you understood https://github.com/crystal-lang/crystal/pull/7498/files#r261847933 – no need to have custom code related to Hash on the expectation anymore.

@asterite
Copy link
Member

asterite commented Mar 3, 2019

Ruby provides <, <=, > and >= for this (subset, subset or equals, superset, superset or equals). Let's do the same.

@dscottboggs
Copy link
Contributor

I don't think it's accurate to say that this closes #7497. That issue was about a totally different question.

@Sija
Copy link
Contributor Author

Sija commented Mar 3, 2019

@asterite Cool, these are way better! How about also delegating Hash#includes?(other : Hash) to other <= self to allow contain expectation to work smoothly with it?

@straight-shoota
Copy link
Member

#includes?(a) is defined as returning true when a is part of the collection. You're proposing an entirely different semantic, which is actually a subset predicate. This should be a different method and would be implemented by #7500.

I'd suggest to close this PR.

@straight-shoota
Copy link
Member

I've given this some thought and maybe we could consider adding an overload Enumerable(T).includes?(collection : Enumerable) which returns true if all elements in collection are included in self. So that's essentially other.all? { |elem| includes?(elem) }.

/cc #7500 (comment)

@asterite
Copy link
Member

If we do that it should be named includes_all?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants