Skip to content

Conversation

@Avital-Fine
Copy link
Contributor

SortedSetAdd currently not supporting GT and LT features.

Also, CH is not supported. @NickCraver Should I add it here in this PR too?

@NickCraver
Copy link
Collaborator

I think we should do all the things at once here given it's the same overloads, yep - @mgravell and I are unsure about the When because the options are more complex/confusing. But we should tackle the CH and INCR at the same time given it's all the same method ultimately. I'm back in action this week so trying to resume PRs tonight.

@Avital-Fine
Copy link
Contributor Author

Great! Glad you're back :)
Are you sure about the INCR option? Because it's exactly like using SortedSetIncrement 😅

@NickCraver
Copy link
Collaborator

Hmm yeah okay let's ignore for now - plan is if someone needs the combo of these args and INCR, then we can add it to SortedSetIncrement down the road :)

@NickCraver
Copy link
Collaborator

@Avital-Fine talking about this in call - can we please try renaming the When to SortedSetWhen which has all the members? Then we don't have the existing When in the new overload - it's an overall change to this [Flags] enum to specify the options.

/cc @mgravell

@Avital-Fine
Copy link
Contributor Author

@Avital-Fine talking about this in call - can we please try renaming the When to SortedSetWhen which has all the members? Then we don't have the existing When in the new overload - it's an overall change to this [Flags] enum to specify the options.

/cc @mgravell

Maybe we can add SortedSetAddFlags and then we can include there CH flag? WDYT?

@Avital-Fine
Copy link
Contributor Author

Also it will be super easy to support INCR

@Avital-Fine Avital-Fine changed the title Support GT and LT in ZADD command Support GT, LT and CH in ZADD command May 13, 2022
@Avital-Fine
Copy link
Contributor Author

Hey @NickCraver!
The PR is Ready

@NickCraver
Copy link
Collaborator

@Avital-Fine ACK! Sorry I've been swamped and not on this hardly at all - @mgravell is going to give compat eyes here :)

@NickCraver NickCraver merged commit 67297e3 into StackExchange:main Jun 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants