Add SetArgs command #1662
Add SetArgs command #1662
Conversation
| @@ -1169,7 +1374,7 @@ var _ = Describe("Commands", func() { | |||
| Expect(val).To(Equal("hello")) | |||
|
|
|||
| Eventually(func() error { | |||
| return client.Get(ctx, "foo").Err() | |||
| return client.Get(ctx, "key").Err() | |||
There was a problem hiding this comment.
I think it should be "key" and not "foo", since we want to test that "key" expired correctly.
The test succeeded because "foo" didn't exist here, so it was always nil.
There was a problem hiding this comment.
Thanks for fixing this 👍
commands.go
Outdated
| } | ||
|
|
||
| // SetWithArgs provides a way to call the SET command with SetArgs arguments. | ||
| func (c cmdable) SetWithArgs(ctx context.Context, key string, value interface{}, a *SetArgs) *StatusCmd { |
There was a problem hiding this comment.
Let's call this SetArgs with we already have such name and it is better to follow one convention.
commands.go
Outdated
| @@ -788,6 +788,50 @@ func (c cmdable) Set(ctx context.Context, key string, value interface{}, expirat | |||
| return cmd | |||
| } | |||
|
|
|||
| // SetArgs provides arguments for the SetWithArgs command. | |||
| // | |||
| // Mode can be `NX` or `XX` or empty. | |||
There was a problem hiding this comment.
This should be a comment on Mode string field. And similar for Zero and Get.
There was a problem hiding this comment.
I improved the readability
commands.go
Outdated
| // When Get is true, the command returns the old value stored at key, or nil when key did not exist. | ||
| type SetArgs struct { | ||
| Mode string | ||
| ExpireAt time.Duration |
There was a problem hiding this comment.
I believe ExpireAt should be called TTL.
And there is no support for EXAT timestamp|PXAT milliseconds-timestamp. II is fine if you decide not to support it in this PR, but I believe we need:
TTL time.Duration // for px/ex
ExpireAt time.Time // for pxat/exat
commands.go
Outdated
| } else { | ||
| args = append(args, "ex", formatSec(ctx, a.ExpireAt)) | ||
| } | ||
| } else if a.ExpireAt == KeepTTL { |
There was a problem hiding this comment.
I would prefer this to be KeepTTL bool field.
if a.KeepTTL { ... }
It is more explicit and we have both TTL and ExplireAt which can be confusing.
|
Thanks. I've left few comments but overall this is looking good. Test coverage is impressing 👍 |
|
Thank you for your review! I'll look at this. |
|
Thanks 👍 I will experiment with API a bit too... |
SetArgs supports all the SET command options.
It is the alternative to the Set function when the user wants more control over the SET options.