Implement GETEX command by benbryant0 · Pull Request #1743 · StackExchange/StackExchange.Redis (original) (raw)

Hey @benbryant0 - looks pretty good, just a few things.

1: I think there should be another API specifically for persistence rather than passing in null. I worked backward a bit and read through the code rather than reading your back & forth, I think 3 methods per sync/async would make the most sense here. Fundamentally the command allows 3 things, persist a given amount of time, persist until a time, and persit indefinitely. So this should either be 1 enum Operated method

StringGetSetExpiry(RedisKey key, ExpiryType type, Timespan? timespan = null, Timestamp? timestamp = null, Commandflags flags = CommandFlags.None)

or just split it out into 3 methods, which IMO is cleaner and will result in fewer paths, and fewer need for the library to toss exceptions. Sort of thinking the null timespan setting it to persist forever is confusing (I might expect it to have some reasonable default value e.g. 60 seconds, or something configured).

2: There's some extra logic to pull out seconds vs milliseconds I don't think necessary, just use milliseconds, I believe Redis will just use Milliseconds regardless, and you're going to send a 64 bit integer no matter what.

3: Very minor stylistic comments (casing and Assertions) in the tests. Looks like there is one missing test to exercise the off-nominal ArgumentException we're adding above.