Fix for #2071: StringSet compatibility by NickCraver · Pull Request #2098 · StackExchange/StackExchange.Redis (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation4 Commits6 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
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.
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.
| // Backwards compatibility overloads: |
| public bool StringSet(RedisKey key, RedisValue value, TimeSpan? expiry, When when) => |
| StringSet(key, value, expiry, false, when, CommandFlags.None); |
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?
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 ` 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.
Reverts #2100
This was aimed to be merged after #2098 landed, my fault. Reverting out for that to happen.
NickCraver added a commit that referenced this pull request
@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.
