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

Rationalize contains matcher #49

Open
joshua-i opened this issue May 15, 2017 · 4 comments
Open

Rationalize contains matcher #49

joshua-i opened this issue May 15, 2017 · 4 comments
Labels
type-enhancement A request for a change that isn't a bug

Comments

@joshua-i
Copy link

Right now, contains works on Iterables, Strings, and Maps. For Iterables, I believe it is equivalent to the anyElement matcher. For String, I think it is equivalent to the matches matcher (in string_matchers). For Maps, it does containsKey (which is surprising on its own), and a new matcher would need to be created.

Maybe contains should be deprecated in favor of the equivalent matchers mentioned above? Or at least perhaps the behavior can be made more consistent. It takes a Matcher for Iterable, but not String or Maps.

@srawlins
Copy link
Member

+1 To me, the implicit containsKey is the big surprise here. Surprises are bad.

@nex3
Copy link
Member

nex3 commented May 17, 2017

I think contains() makes a lot of sense as-is for strings and iterables. I don't have a problem with the same name being used for multiple types; it doesn't seem meaningfully different to me than having both Iterable.contains() and String.contains() exist.

I do agree that contains() meaning containsKey() for a map is bizarre. I'd be fine with adding a containsKey() function and adding a comment indicating that contains() for maps is deprecated.

@nex3 nex3 added the type-enhancement A request for a change that isn't a bug label May 17, 2017
@joshua-i
Copy link
Author

Here's why I still don't like it.

  1. It's not great to have an API with redundant functionality. Is there any difference between using contains on an iterable vs. anyElement? Will you get rid of anyElement then?
  2. When Could Matcher be changed to be typed as Matcher<T>? #37 is solved, what will T be for contains? Will it be forced into dynamic until union types are implemented?
  3. If we're insisting that it should work on both String and Iterable because they both have a function named "contains", then can it at least be implemented with just a dynamic invocation of contains? It would then actually work on anything with contains (e.g. Streams), rather than try to be "smart" for an Iterable, handling Matcher args, which is already taken care of by anyElement.

@nex3
Copy link
Member

nex3 commented May 17, 2017

It's not great to have an API with redundant functionality. Is there any difference between using contains on an iterable vs. anyElement? Will you get rid of anyElement then?

I suppose I'd be okay with marking anyElement() as deprecated.

When #37 is solved, what will T be for contains? Will it be forced into dynamic until union types are implemented?

Hopefully we'll also have overloads by that point, so we'll just define two overloads. Equivalently, we could type contains() as the union type Pattern -> Matcher<String> | Matcher<T> -> Matcher<Iterable<T>> | T -> Matcher<Iterable<T>>.

If we're insisting that it should work on both String and Iterable because they both have a function named "contains", then can it at least be implemented with just a dynamic invocation of contains? It would then actually work on anything with contains (e.g. Streams), rather than try to be "smart" for an Iterable, handling Matcher args, which is already taken care of by anyElement.

This seems much less useful than the current behavior, since the only widely-used types that implement contains() in practice are Iterables and Strings.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests

3 participants