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

Support expires_in for List, Set, UniqueList, OrderedSet #143

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

heka1024
Copy link
Contributor

@heka1024 heka1024 commented Feb 1, 2024

Extending the approach taken by #142, now users can set expires_in for List, Set, UniqueList, OrderedSet

@bb
Copy link

bb commented Mar 1, 2024

Hi @heka1024, thanks for this PR. I was looking for expiration on sets and happy to find your work.

Do you plan to also add expiration to the ActiveModel attributes via the Kredis::Attributes concern? You can grab the code (without extra tests for now) here: https://github.com/bb/kredis/blob/support-ttl/lib/kredis/attributes.rb, just cherry-pick the latest commit.

Also, I'm afraid to get this PR merged, some documentation is needed.

@heka1024
Copy link
Contributor Author

heka1024 commented Mar 2, 2024

@bb Thanks. I'll chery pick your commit and supplement documentation.

@heka1024
Copy link
Contributor Author

heka1024 commented Mar 3, 2024

@bb I just cherry-picked your commit and add some test with documentation. Could you review it?

@bb
Copy link

bb commented Mar 3, 2024

Hey @heka1024, thanks a lot!

The tests you already added in the last commits look good to me. I'm afraid there's no test for ordered set yet.

Docs look also good, I don't think exhaustive examples of all params is needed here, so I'd leave it as is. Keep in mind I'm not a maintainer here, so in the end it's not my decision.

@bb
Copy link

bb commented Mar 4, 2024

Thanks for adding the missing test @heka1024. LGTM.
As said, I'm no maintainer, so I cannot formally approve it.

Maybe @dhh (or another maintainer) can have a look now because his concern about test failures in #133 (comment) is addressed now.

@balbesina
Copy link

any reasons why this list doesn't include Hash ?

@heka1024
Copy link
Contributor Author

@balbesina. I just added expiration behavior in hash. I rarely used Redis's hash, I just forgot it 😅.

@heka1024
Copy link
Contributor Author

Hi, @dhh. Could you review this? I fixed #133 (comment) failing tests.

@roharon
Copy link

roharon commented May 15, 2024

Although this is a different topic, have you considered adding an expires_at option?
I believe it would be beneficial for [product or feature] to also support expires_at. If it hasn't been considered yet, I'm thinking about adding expires_at and would like to follow up on it.

relates rails/rails#41831

@heka1024
Copy link
Contributor Author

@roharon It'll be nice if we can set expires_at! Could you create PR for it?

@roharon
Copy link

roharon commented May 20, 2024

@heka1024 Cool, I'll work on it.

@thiagopradi
Copy link

@heka1024 - Please let me know if you need any help with this PR to get it merged. It will be useful for me too.

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.

6 participants