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

Allow passing Entry subclasses to Map factory methods #51

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kilink
Copy link

@kilink kilink commented Jun 5, 2023

Change the factory methods to accept a covariant type (e.g., Iterable<? extends Map.Entry>), which allows passing Iterables of UnEntry.

Note: perhaps I'm just using the library wrong, but I ran into this issue when I was attempting to merge two maps. It seemed to me that I should be able to do this:

PersistentTreeMap.of(map.concat(map2));

While I know it's doable like so, to me it seems like an unnecessary limitation, and this form is more verbose:

map.concat(map2).toImSortedMap(Equator.defaultComparator(), entry -> entry);

(Would be happy to be shown a better way to merge two maps).

Another consequence of making it covariant is that you can now pass a Persistent map instance to the factory method.

Change the factory methods to accept a covariant type (e.g., Iterable<? extends Map.Entry>),
which allows passing Iterables of UnEntry.
@GlenKPeterson
Copy link
Owner

Thank you. What you're asking for seems to make sense. I appreciate that you matched the code style and added unit tests. I'm sorry to take so long to even start looking at this.

I had a few nagging memories that turned out to be red herrings. The one that I'm still concerned about is type inference in Java. It should probably all be OK though. I need more time to try it out on my machine before merging, but at this point, I do plan to merge your changes.

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

Successfully merging this pull request may close these issues.

2 participants