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

Restrict generic key type to only those supported #371

Merged

Conversation

pete-woods
Copy link
Contributor

@pete-woods pete-woods commented Jan 16, 2024

Without this, you can specify key types that aren't supported and then have to deal with panics, rather than more pleasant compile-time warnings.

@CLAassistant
Copy link

CLAassistant commented Jan 16, 2024

CLA assistant check
All committers have signed the CLA.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

mangalaman93
mangalaman93 previously approved these changes Jan 16, 2024
@mangalaman93
Copy link
Member

Thanks for the PR. The only problem that I see with this is that any new type addition requires changes to the code and upgrading ristretto.

@pete-woods
Copy link
Contributor Author

pete-woods commented Jan 16, 2024

Thanks for the PR. The only problem that I see with this is that any new type addition requires changes to the code and upgrading ristretto.

I believe this is already the case (see linked code): https://github.com/dgraph-io/ristretto/blob/main/z/z.go#L55

Also I put the type next to the key mapping func, so it's clear they both need updating in lockstep.

z/z.go Show resolved Hide resolved
z/z.go Show resolved Hide resolved
@mangalaman93 mangalaman93 merged commit dae2557 into hypermodeinc:main Jan 16, 2024
5 checks passed
@pete-woods pete-woods deleted the more-restrictive-generics branch January 16, 2024 10:15
@pete-woods
Copy link
Contributor Author

Thanks for your time! 🙇

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

Successfully merging this pull request may close these issues.

3 participants