-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Fix for #2071: StringSet compatibility #2098
Conversation
I missed an overload case being an idiot - adding the missing source break for the case in #2071. ...also fixing KeyTouch ordering while in here. Note that adding `CommandFlags` back optional seems like a quick fix and I did try that route, but in a full test suite here it became apparent that created other ambiguous overload cases, so went this route.
@@ -2945,6 +2945,9 @@ public Task<long> StringLengthAsync(RedisKey key, CommandFlags flags = CommandFl | |||
return ExecuteAsync(msg, ResultProcessor.Int64); | |||
} | |||
|
|||
// Backwards compatibility overloads: | |||
public bool StringSet(RedisKey key, RedisValue value, TimeSpan? expiry, When when) => | |||
StringSet(key, value, expiry, false, when, CommandFlags.None); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can't we just make flags
on the overload below option with a default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately I don't think so - the problem is then usage of RedisKey key, RedisValue value, TimeSpan? expiry, When when
becomes ambiguous between the 2 methods. This was my first play based on your suggestion and my thoughts too, but couldn't make it work.
This does a few things globally to the interfaces: - De-dupes `<remarks>` since evidently past the first one doesn't count/render - Links our redis command links (and all others) so they're easily clickable! - Moves a few types to proper class files - In places sync/async methods are adjacent, utilizes `<inheritdoc cref="" /> to de-dupe - ...and some other misc URL cleanup throughout. In general: docs only change - I think we should merge this as-is to help PRs coming in, then I'll continue to iterate on docs.
Ah crap, reversed the ordering here and merged docs branch before this went to main, dammit. |
@mgravell apologies for noise (I goofed on PR layering), here's the issue with the simple fix which I think we have to assume may be in use: If we can go simpler I'm all for it, just didn't see a way to account for current possible uses. |
I missed an overload case being an idiot - adding the missing source break for the case in #2071.
...also fixing KeyTouch ordering while in here.
Note that adding
CommandFlags
back optional seems like a quick fix and I did try that route, but in a full test suite here it became apparent that created other ambiguous overload cases, so went this route.