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

ContainsAll is confusing #20

Open
marcglasberg opened this issue May 31, 2020 · 3 comments
Open

ContainsAll is confusing #20

marcglasberg opened this issue May 31, 2020 · 3 comments
Labels
type-enhancement A request for a change that isn't a bug

Comments

@marcglasberg
Copy link

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
Copy link
Member

lrhn commented Sep 4, 2020

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
Copy link
Member

lrhn commented May 7, 2021

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.

mit-mit added a commit to mit-mit/characters that referenced this issue Jul 26, 2021
@lrhn lrhn added the type-enhancement A request for a change that isn't a bug label Aug 4, 2021
@lrhn
Copy link
Member

lrhn commented Aug 4, 2021

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.

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

2 participants