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

generics: widen map key constraint to 'comparable' #29

Merged
merged 2 commits into from
Dec 20, 2022

Conversation

laher
Copy link
Contributor

@laher laher commented Dec 4, 2022

Addressing #25 (second attempt)

As suggested by @anacrolix after my previous attempt (#26):

I gave it a spin, it didn't work for the underlying user (although the stm code passed). I think the type can be widened for SortedMap (and any other variants too). Again, because comparers are manually given, the restriction on types belong to those implementations. I'm pretty sure it works for the builtin comparers too, as those use type sets to check for orderable types anyway. Short answer: I think K comparable can be used everywhere in immutable.

So I used comparable, but under the hood it only supports strings, the various integer types, and derived types.

As before, the defaultComparer panics if you try to use any other key type. You just need to write your own Comparer for other types.

@benbjohnson
Copy link
Owner

@laher Sorry for the late reply. AFAICT this looks ok. @anacrolix what do you think?

@anacrolix
Copy link

Give me a few days to try it out.

@BarrensZeppelin
Copy link
Contributor

I've been using this for a while without issues. 👍

@benbjohnson benbjohnson merged commit 8932b99 into benbjohnson:master Dec 20, 2022
@benbjohnson
Copy link
Owner

Thanks @laher. I went ahead and merged. If anyone has any objections or changes, go ahead and open a new PR. 👍

@laher laher deleted the map-keys-comparable branch December 20, 2022 21:54
@anacrolix
Copy link

This did solve the issue, I used it in anacrolix/stm@32174bf and anacrolix/dht@88ee338.

@laher
Copy link
Contributor Author

laher commented Dec 21, 2022

Nice one. I just noticed that I didn't update the readme. I can PR it a bit later, and then maybe we need another release?

@benbjohnson
Copy link
Owner

I went ahead and cut a v0.4.1 release: https://github.com/benbjohnson/immutable/releases/tag/v0.4.1

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.

None yet

4 participants