kredis icon indicating copy to clipboard operation
kredis copied to clipboard

Support `expires_in` for `List`, `Set`, `UniqueList`, `OrderedSet`

Open heka1024 opened this issue 2 years ago • 14 comments

Extending the approach taken by https://github.com/rails/kredis/pull/142, now users can set expires_in for List, Set, UniqueList, OrderedSet

heka1024 avatar Feb 01 '24 03:02 heka1024

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.

bb avatar Mar 01 '24 22:03 bb

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

heka1024 avatar Mar 02 '24 16:03 heka1024

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

heka1024 avatar Mar 03 '24 15:03 heka1024

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 avatar Mar 03 '24 19:03 bb

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 https://github.com/rails/kredis/pull/133#issuecomment-1872184287 is addressed now.

bb avatar Mar 04 '24 10:03 bb

any reasons why this list doesn't include Hash ?

balbesina avatar Mar 30 '24 08:03 balbesina

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

heka1024 avatar Mar 31 '24 06:03 heka1024

Hi, @dhh. Could you review this? I fixed https://github.com/rails/kredis/pull/133#issuecomment-1872184287's failing tests.

heka1024 avatar May 13 '24 05:05 heka1024

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 https://github.com/rails/rails/pull/41831

roharon avatar May 15 '24 17:05 roharon

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

heka1024 avatar May 20 '24 14:05 heka1024

@heka1024 Cool, I'll work on it.

roharon avatar May 20 '24 14:05 roharon

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

thiagopradi avatar Jul 31 '24 00:07 thiagopradi