Support LMOVE by Avital-Fine · Pull Request #2065 · 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

Conversation19 Commits13 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 }})

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this! I left comments inline - note that #2041 is a major change we wanted to get in before tons of PRs here (per #2055) and will make merging a bit difficult. I wanted to throw a note in here in case unaware. I'll do me best to help merge main into these after NRTs lands.

Co-authored-by: Nick Craver nrcraver@gmail.com

@Avital-Fine for tests, check out the format Skip.IfMissingFeature(conn, nameof(RedisFeatures.PushMultiple), f => f.PushMultiple); - we need to so similar for LMOVE there for testing against older servers :)

Co-authored-by: Nick Craver nrcraver@gmail.com

Thanks for this! I left comments inline - note that #2041 is a major change we wanted to get in before tons of PRs here (per #2055) and will make merging a bit difficult. I wanted to throw a note in here in case unaware. I'll do me best to help merge main into these after NRTs lands.

Thank you! hope the merge won't be too bad 😅

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few tweaks/removals then I think we're good to go - thanks for this!


public RedisValue ListMove(RedisKey sourceKey, RedisKey destinationKey, ListSide whereFrom, ListSide whereTo, CommandFlags flags = CommandFlags.None)
{
var msg = Message.Create(Database, flags, RedisCommand.LMOVE, sourceKey, destinationKey, whereFrom == ListSide.Left ? "Left" : "right", whereTo == ListSide.Left ? "Left" : "right");

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should be consistent in Left vs right here :)

Skip.IfMissingFeature(conn, nameof(RedisFeatures.PushMultiple), f => f.PushMultiple);
var db = conn.GetDatabase();
RedisKey key = "testlist";
RedisKey key = Me();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for these fixes btw!

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure!

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Looking good

Support LMOVE by Avital-Fine · Pull Request #2065 · StackExchange/StackExchange.Redis (original) (raw)

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Labels