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

Add v2 with generics #43

Merged
merged 4 commits into from
Feb 20, 2024
Merged

Add v2 with generics #43

merged 4 commits into from
Feb 20, 2024

Conversation

paskal
Copy link
Collaborator

@paskal paskal commented Feb 18, 2024

Redis cache creation will return an error if used with a type which is not a string or not based on a string.

Resolves #31 and #39.

@paskal paskal requested a review from umputun as a code owner February 18, 2024 18:37
@paskal paskal changed the title add v2 with generics Add v2 with generics Feb 18, 2024
@paskal paskal marked this pull request as draft February 18, 2024 18:37
@paskal paskal force-pushed the paskal/generics branch 2 times, most recently from a12379d to 0da3b2b Compare February 18, 2024 21:11
@paskal paskal marked this pull request as ready for review February 18, 2024 21:11
@paskal paskal force-pushed the paskal/generics branch 2 times, most recently from 0b1bf28 to b74880a Compare February 18, 2024 21:28
v2/go.mod Outdated Show resolved Hide resolved
@umputun
Copy link
Member

umputun commented Feb 18, 2024

Looking good. I'm not a big fan of using reflection for Redis, but it's likely the right approach in this situation. Please ping me once you're done with everything, and I'll take a closer look.

@paskal
Copy link
Collaborator Author

paskal commented Feb 18, 2024

@umputun, everything is ready for review. It took some time for me to figure out how to combine two coverage profiles into one for a proper coverage report.

The code in github.com/hashicorp/golang-lru/v2/expirable
was provided by me and was inspired by the code here.
Copy link
Member

@umputun umputun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of things:

  • lint/style: imports are not sorted properly, one of file has import twice
  • attempt to go get -u ./... brings redis's lib update, and it seems to be incompatible with our code

@paskal
Copy link
Collaborator Author

paskal commented Feb 19, 2024

The Redis problem seems to be redis/go-redis#2914. It should be fixed upstream, as it affects a lot of cases.

@paskal paskal requested a review from umputun February 19, 2024 18:28
Copy link
Member

@umputun umputun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@paskal paskal merged commit c8160d1 into master Feb 20, 2024
2 of 4 checks passed
@paskal paskal deleted the paskal/generics branch February 20, 2024 18:24
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.

Modernize library, introduce generic interface
2 participants