dart-lang / core

This repository is home to core Dart packages.
https://pub.dev/publishers/dart.dev
BSD 3-Clause "New" or "Revised" License
17 stars 4 forks source link

ContainsAll is confusing #377

Open marcglasberg opened 4 years ago

marcglasberg commented 4 years ago

In the declaration:

bool contains(Object Character); 

The identifier Character should be lowercase ➜ character

But, even so, the analogy with a collection of characters is not a good one. If you say containsAll in a Set it doesn't mean a sequence, but the individual elements. Here it means a subsequence: the characters in the exact order, with no other characters between them.

I'd remove this method and leave only the contains method, which would accept a single character or multiple characters. You say it should not accept it /// because then it is not a single element of this [Iterable] of characters. You don't want to break the Iterable's contract, but so what? I don't see how anyone could use it wrong if you merge both methods into a single one.

Still, if you don't like that I'd change the declaration of both methods to:

bool contains(Object singleChar); 
bool containsSequence(Characters sequence); 

That's much clearer and easier to use.

lrhn commented 4 years ago

We cannot make contains do what we want, because it must accept a String (because Characters is an Iterable<String>). Allowing it to accept either a String (which must be a single character) or Characters object which can represent multiple characters, is going to be hard to type and probably confusing. That said, contains already takes Object as argument, so it's untypable to begin with. If we allows it to accept a Characters object too, and match multiple consecutive clusters, then we should probably also allow a string containing multiple clusters to match. It would still say true for all the elements, but it would say true for other inputs too. inputs which do not represent a single grapheme cluster. So, it's possibly acceptable.

The containsAll name can be confusing.

lrhn commented 3 years ago

I've become more inclined to allow Characters as arguments as well, and make it work like containsAll in that case. Still not a good idea to allow strings containing multiple characters. The biggest problem is that we can't type it. For that, I'd prefer to change it to contains(covariant String ...) instead, and then it can't work with Characters.

Changing containsAll to something else is a breaking change, but I'm willing to entertain it for version 2.0.

lrhn commented 3 years ago

The contains method now takes covariant String as argument in the interface, which should catch attempts to call it with a Characters. It still takes Object? in the implementation, so up-casting to Iterable<Object?> is safe. I don't think we can do much better for contains than that while still implementing Iterable<String>.

The containsAll might we worth renaming if we make a major version increment.