Skip to content

Restrict generic key type to only those supported#371

Merged
mangalaman93 merged 1 commit intodgraph-io:mainfrom
pete-woods:more-restrictive-generics
Jan 16, 2024
Merged

Restrict generic key type to only those supported#371
mangalaman93 merged 1 commit intodgraph-io:mainfrom
pete-woods:more-restrictive-generics

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
Contributor

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.

@mangalaman93 mangalaman93 merged commit dae2557 into dgraph-io:main Jan 16, 2024
@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