Skip to content

Node: add LMOVE - #2002

Merged
Yury-Fridlyand merged 46 commits into
valkey-io:mainfrom
Bit-Quill:node/integ_cyip10_lmovefinal
Jul 25, 2024
Merged

Yury-Fridlyand merged 46 commits into
valkey-io:mainfrom
Bit-Quill:node/integ_cyip10_lmovefinal

Conversation

@cyip10

@cyip10 cyip10 commented Jul 23, 2024

Copy link
Copy Markdown
Contributor

No description provided.

Signed-off-by: Chloe Yip <chloe.yip@improving.com>
@cyip10
cyip10 requested a review from a team as a code owner July 23, 2024 18:27
@cyip10 cyip10 changed the title Node: LMOVE neifjcbfgkhdcfjigleutgcrguevllrlibjubinghdjl Node: add LMOVE Jul 23, 2024
@cyip10 cyip10 added the node 🐢 Node.js wrapper label Jul 23, 2024
Signed-off-by: Chloe Yip <chloe.yip@improving.com>

@Yury-Fridlyand Yury-Fridlyand left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Add changelog

Comment thread node/src/BaseClient.ts Outdated
Comment thread node/src/BaseClient.ts Outdated
Comment thread node/src/BaseClient.ts Outdated
Comment thread node/src/Commands.ts Outdated
Comment thread node/src/Transaction.ts Outdated
Comment thread node/src/Transaction.ts Outdated
Comment thread node/tests/SharedTests.ts Outdated
cyip10 added 4 commits July 23, 2024 16:02
Signed-off-by: Chloe Yip <chloe.yip@improving.com>
Signed-off-by: Chloe Yip <chloe.yip@improving.com>
Signed-off-by: Chloe Yip <chloe.yip@improving.com>
Signed-off-by: Chloe Yip <chloe.yip@improving.com>
Comment thread node/src/commands/ListDirection.ts Outdated
Comment thread node/tests/SharedTests.ts Outdated
cyip10 and others added 4 commits July 24, 2024 11:17
Signed-off-by: Chloe Yip <chloe.yip@improving.com>
Signed-off-by: Chloe Yip <168601573+cyip10@users.noreply.github.com>
Signed-off-by: Chloe Yip <chloe.yip@improving.com>
Signed-off-by: Chloe Yip <chloe.yip@improving.com>
Comment thread node/src/BaseClient.ts Outdated
Comment thread node/src/BaseClient.ts Outdated
Comment thread node/src/Transaction.ts Outdated
Comment thread node/tests/SharedTests.ts
Comment thread node/tests/SharedTests.ts Outdated
Comment thread node/tests/SharedTests.ts Outdated
Comment thread node/tests/SharedTests.ts Outdated
Comment thread node/tests/TestUtilities.ts Outdated
cyip10 added 14 commits July 24, 2024 12:52
Signed-off-by: Chloe Yip <chloe.yip@improving.com>
Signed-off-by: Chloe Yip <chloe.yip@improving.com>
Signed-off-by: Chloe Yip <chloe.yip@improving.com>
Signed-off-by: Chloe Yip <chloe.yip@improving.com>
Signed-off-by: Chloe Yip <chloe.yip@improving.com>
Signed-off-by: Chloe Yip <chloe.yip@improving.com>
Signed-off-by: Chloe Yip <chloe.yip@improving.com>
Signed-off-by: Chloe Yip <chloe.yip@improving.com>
Signed-off-by: Chloe Yip <chloe.yip@improving.com>
Signed-off-by: Chloe Yip <chloe.yip@improving.com>
Signed-off-by: Chloe Yip <chloe.yip@improving.com>
Signed-off-by: Chloe Yip <chloe.yip@improving.com>
Signed-off-by: Chloe Yip <chloe.yip@improving.com>
Signed-off-by: Chloe Yip <chloe.yip@improving.com>
cyip10 added 7 commits July 24, 2024 13:50
Signed-off-by: Chloe Yip <chloe.yip@improving.com>
Signed-off-by: Chloe Yip <chloe.yip@improving.com>
Signed-off-by: Chloe Yip <chloe.yip@improving.com>
…movefinal' into node/integ_cyip10_lmovefinal
Signed-off-by: Chloe Yip <chloe.yip@improving.com>
Signed-off-by: Chloe Yip <chloe.yip@improving.com>
Signed-off-by: Chloe Yip <chloe.yip@improving.com>

@Yury-Fridlyand Yury-Fridlyand left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Please add version check to the test

Comment thread node/src/BaseClient.ts Outdated
Comment thread node/src/Transaction.ts Outdated
Signed-off-by: Chloe Yip <chloe.yip@improving.com>
Comment thread node/src/BaseClient.ts Outdated
Comment thread node/src/Transaction.ts Outdated

@yipin-chen yipin-chen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM - don't forget to add version check in SharedTests.

Signed-off-by: Chloe Yip <chloe.yip@improving.com>

@GumpacG GumpacG left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

SharedTest still needs version checks but looks good otherwise.

Signed-off-by: Chloe Yip <chloe.yip@improving.com>
Comment thread node/tests/TestUtilities.ts Outdated
Signed-off-by: Chloe Yip <chloe.yip@improving.com>
Comment thread node/src/BaseClient.ts Outdated
Comment thread node/src/Transaction.ts
*
* since Valkey version 6.2.0.
*/
public lmove(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
public lmove(
public async lmove(

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It is not async in transaction I think

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

async on the exec() call!

Comment thread node/src/BaseClient.ts
* @param source - The key to the source list.
* @param destination - The key to the destination list.
* @param whereFrom - The {@link ListDirection} to remove the element from.
* @param whereTo - The {@link ListDirection} to add the element to.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I dont remember what we said, does @link works? or is it a temporary template until we decide what we should do?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It should, according to docs: https://tsdoc.org/pages/tags/link/

cyip10 added 2 commits July 25, 2024 10:41
Signed-off-by: Chloe Yip <chloe.yip@improving.com>
Signed-off-by: Chloe Yip <chloe.yip@improving.com>
@cyip10
cyip10 force-pushed the node/integ_cyip10_lmovefinal branch from d3eabef to 3e5c94d Compare July 25, 2024 19:04
Signed-off-by: Chloe Yip <168601573+cyip10@users.noreply.github.com>
Comment thread node/tests/TestUtilities.ts Outdated
Signed-off-by: Chloe Yip <chloe.yip@improving.com>

@acarbonetto acarbonetto left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

lgtm

@Yury-Fridlyand
Yury-Fridlyand merged commit 1c70afb into valkey-io:main Jul 25, 2024
@Yury-Fridlyand
Yury-Fridlyand deleted the node/integ_cyip10_lmovefinal branch July 25, 2024 20:38
affonsov pushed a commit that referenced this pull request Aug 21, 2026
* implement lmove

Signed-off-by: Chloe Yip <chloe.yip@improving.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

node 🐢 Node.js wrapper

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants