[BREAKING] support generic API #321

aryehlev · 2022-12-11T17:50:18Z

No description provided.

CLAassistant · 2022-12-11T17:50:22Z

All committers have signed the CLA.

CLAassistant · 2022-12-11T17:50:22Z

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.}

GRbit · 2023-03-20T15:29:17Z

@aryehlev since there is no reaction from the repository maintainer, have you thought about updating your fork with a description with a headline like:

This is a fork of github.com/dgraph-io/ristretto but with **Magick of Generics**

I think it's an incredibly convenient feature to have a ristretto with generics, so it has the potential of getting some stars.

mangalaman93 · 2023-03-20T16:05:32Z

Thanks for getting this to our attention. I will review the PR now. Please sign the CLA when you can.

mangalaman93

I think we need to figure out if we want to use pointer to K & V everywhere first. I can review the rest of the PR then. Let me know what you think. Thanks for the PR.

cache.go

GRbit · 2023-03-20T17:54:47Z

@mangalaman93 thank you for your prompt response and the review.

@aryehlev I'm not hasting you at all, but since your PR was created a while ago, there is a chance that your situation and intentions have changed and you no longer want to spend time on this. In this case, please let us know. Maybe someone else will appear who wants to work on the PR.

mangalaman93 · 2023-03-20T20:25:12Z

@GRbit I think this is a useful PR, worth including. I am gonna slowly look at other PRs too and figure out our release plan. This would a breaking change, so it can only go into next major release etc.

In terms of whether the args should be pointers, I think you are right. They shouldn't be pointers. I got confused seeing all these nil checks in the existing code. Could you just go through the rest of my comments and ensure that we are not doing the nil checks anywhere. I can review your PR then again.

While we can get the PR ready, let me figure out a release plan too. This could take some time though, just FYI.

aryehlev · 2023-03-20T20:40:25Z

@GRbit @mangalaman93

Hey, thank you for the comments.
I can definitely look at this and do the necessary fixes later. I think I did not use pointers for a reason.

It is definitely a major change and will only work with go 1.18 and up so I am not sure how you will want to release this.

GRbit · 2023-03-20T21:06:43Z

This would be a breaking change, so it can only go into the next major release etc.

It is definitely a major change and will only work with go 1.18 and up so I am not sure how you will want to release this.

Luckily it's 1.12 already, so versions could e used to solve this issue. Though, people who update their libraries as a rule of thumb update Go version as well.

This could take some time though, just FYI.

It's great to see activity here, even if it takes time. Thank you for all your time and efforts, it's really appreciated.

mangalaman93 · 2023-03-20T21:34:14Z

We have upgraded badger and dgraph to Go 1.19, we should be able to do that for ristretto too while we do the next major release. It should be fine to test the PR with the same.

cache.go

mangalaman93 · 2023-08-31T08:05:27Z

Let me know if you are facing any challenge resolving conflicts.

# Conflicts: # cache.go # cache_test.go # go.mod # policy.go # ttl.go # z/z.go

aryehlev · 2023-08-31T09:33:46Z

i think i now resolved all conflicts, please go over the code if u can

mangalaman93

A few more comments. I will ask more engineers to review this PR.

store.go

z/z.go

# Conflicts: # cache.go

z/z.go

mangalaman93

Hi @aryehlev, just a couple of minor comments and we are good to merge. Thanks for all the work.

policy.go

mangalaman93

Looks good to me now! Thank you so much for all the work. I see one typo/spell error, I thought you had fixed it. Could you check that again please? I also want to get more reviews on this, let me see who is available.

store.go

GRbit · 2023-09-04T14:10:36Z

I would like to congratulate @aryehlev on getting this PR approved and thank you both for your work! This was a long PR and it was nice to see it get this far!

aryehlev added 8 commits November 25, 2022 23:00

add generics.

fe3efab

change module name.

4e36ce4

ecxport key type

bd7c08e

changeName

f92fd57

fix nils and make key type comparable.

40b7ca8

change nils

f1fb7b7

try cahnge 1

8ec1534

replace names and fix tests to work with generics.

d388a7e

aryehlev requested review from akon-dey, gcxml, joshua-goldstein and skrdgraph as code owners December 11, 2022 17:50

mangalaman93 requested changes Mar 20, 2023

View reviewed changes

aryehlev added 3 commits March 21, 2023 19:33

Merge remote-tracking branch 'origin/main'

e3ec3dc

fix comments on PR and also fix KeyToHash to be able to deal with nils .

25500b8

remove random change and make version go 1.19 fo performance.

fc1ddb8

aryehlev commented Mar 21, 2023

View reviewed changes

cache.go Outdated Show resolved Hide resolved

set the keyToHash function.

fd222a1

mangalaman93 reviewed Aug 31, 2023

View reviewed changes

cache.go Outdated Show resolved Hide resolved

cache.go Show resolved Hide resolved

change keyToHash function to take K type

23c117a

aryehlev requested a review from harshil-goel as a code owner August 31, 2023 06:51

aryehlev added 3 commits August 31, 2023 10:06

merge cache.go

7c98d35

merge policy.go

cf986a0

merge ttl.go

4ef9df4

aryehlev added 3 commits August 31, 2023 12:21

Merge remote-tracking branch 'originFar/main'

6a17a19

# Conflicts: # cache.go # cache_test.go # go.mod # policy.go # ttl.go # z/z.go

resolve merge changes.

a8fffe8

merge cache_test.go.

e656080

aryehlev added 2 commits August 31, 2023 13:03

fix lint ci.

0d4debf

align tests.

2af8be1

mangalaman93 requested changes Aug 31, 2023

View reviewed changes

store.go Outdated Show resolved Hide resolved

z/z.go Outdated Show resolved Hide resolved

mangalaman93 changed the title ~~feat(Generics): add generics.~~ [BREAKING] support generic API Aug 31, 2023

aryehlev added 5 commits September 1, 2023 08:09

Merge remote-tracking branch 'originFar/main'

ad223b0

put back regular config.KeyToHash.

d59bbcf

if key to hash isnt working return error instead of fatal.

0a3c1dc

Merge remote-tracking branch 'originFar/main'

a052959

# Conflicts: # cache.go

merge with main.

91a06d8

mangalaman93 reviewed Sep 2, 2023

View reviewed changes

z/z.go Outdated Show resolved Hide resolved

mangalaman93 requested changes Sep 4, 2023

View reviewed changes

policy.go Outdated Show resolved Hide resolved

aryehlev added 2 commits September 4, 2023 10:38

fix comment spacing.

d2fba36

put keyToHash without function getter.

f7faa68

mangalaman93 previously approved these changes Sep 4, 2023

View reviewed changes

store.go Outdated Show resolved Hide resolved

fix typo.

e5e54ea

aryehlev dismissed mangalaman93’s stale review via e5e54ea September 4, 2023 08:39

mangalaman93 approved these changes Sep 4, 2023

View reviewed changes

mangalaman93 merged commit e2027d3 into hypermodeinc:main Sep 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BREAKING] support generic API #321

[BREAKING] support generic API #321

aryehlev commented Dec 11, 2022 •

edited by mangalaman93

Loading

CLAassistant commented Dec 11, 2022 •

edited

Loading

CLAassistant commented Dec 11, 2022

GRbit commented Mar 20, 2023

mangalaman93 commented Mar 20, 2023

mangalaman93 left a comment

GRbit commented Mar 20, 2023

mangalaman93 commented Mar 20, 2023

aryehlev commented Mar 20, 2023

GRbit commented Mar 20, 2023

mangalaman93 commented Mar 20, 2023

mangalaman93 commented Aug 31, 2023

aryehlev commented Aug 31, 2023

mangalaman93 left a comment

mangalaman93 left a comment

mangalaman93 left a comment

GRbit commented Sep 4, 2023

[BREAKING] support generic API #321

[BREAKING] support generic API #321

Conversation

aryehlev commented Dec 11, 2022 • edited by mangalaman93 Loading

CLAassistant commented Dec 11, 2022 • edited Loading

CLAassistant commented Dec 11, 2022

GRbit commented Mar 20, 2023

mangalaman93 commented Mar 20, 2023

mangalaman93 left a comment

Choose a reason for hiding this comment

GRbit commented Mar 20, 2023

mangalaman93 commented Mar 20, 2023

aryehlev commented Mar 20, 2023

GRbit commented Mar 20, 2023

mangalaman93 commented Mar 20, 2023

mangalaman93 commented Aug 31, 2023

aryehlev commented Aug 31, 2023

mangalaman93 left a comment

Choose a reason for hiding this comment

mangalaman93 left a comment

Choose a reason for hiding this comment

mangalaman93 left a comment

Choose a reason for hiding this comment

GRbit commented Sep 4, 2023

aryehlev commented Dec 11, 2022 •

edited by mangalaman93

Loading

CLAassistant commented Dec 11, 2022 •

edited

Loading