search

dart-lang / test

A library for writing unit tests in Dart.
https://pub.dev/packages/test
496 stars 214 forks source link

Rationalize contains matcher #2360

Open joshua-i opened 7 years ago

joshua-i commented 7 years ago

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 commented 7 years ago

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

nex3 commented 7 years ago

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.

joshua-i commented 7 years ago

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 dart-lang/matcher#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 commented 7 years ago

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 dart-lang/matcher#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.