Skip to content
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

Node: add ZUNIONSTORE #2145

Merged
merged 14 commits into from
Aug 19, 2024

Conversation

cyip10
Copy link
Collaborator

@cyip10 cyip10 commented Aug 15, 2024

original PR written by @adarovadya
https://valkey.io/commands/zunionstore/

  • addressed remaining comments.

Signed-off-by: Chloe Yip <[email protected]>
@cyip10 cyip10 added the node Node.js wrapper label Aug 15, 2024
@cyip10 cyip10 requested a review from a team as a code owner August 15, 2024 19:47
Copy link
Collaborator

@Yury-Fridlyand Yury-Fridlyand left a comment

Choose a reason for hiding this comment

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

Please add cross slot test

node/src/BaseClient.ts Outdated Show resolved Hide resolved
node/src/BaseClient.ts Outdated Show resolved Hide resolved
node/src/BaseClient.ts Outdated Show resolved Hide resolved
node/src/BaseClient.ts Outdated Show resolved Hide resolved
node/src/BaseClient.ts Outdated Show resolved Hide resolved
node/src/BaseClient.ts Outdated Show resolved Hide resolved
node/src/Transaction.ts Show resolved Hide resolved
node/src/BaseClient.ts Show resolved Hide resolved
@cyip10 cyip10 force-pushed the node/integ_cyip10_zunionstore branch from 8972411 to d3aab02 Compare August 15, 2024 23:15
node/src/BaseClient.ts Show resolved Hide resolved
node/src/BaseClient.ts Show resolved Hide resolved
node/src/BaseClient.ts Outdated Show resolved Hide resolved
node/src/Transaction.ts Show resolved Hide resolved
node/src/BaseClient.ts Show resolved Hide resolved
node/src/Transaction.ts Show resolved Hide resolved
node/src/Transaction.ts Show resolved Hide resolved
`zunionstore test_%p`,
async (protocol) => {
await runTest(async (client: BaseClient) => {
await zunionStoreBasicTest(client);
Copy link
Collaborator

Choose a reason for hiding this comment

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

why did you choose to put everything into a single test case? Better to split this into separate test cases that can be run separately.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It was Adar's desicion and I asked to split it (I keep asking)

Copy link
Collaborator

@Yury-Fridlyand Yury-Fridlyand left a comment

Choose a reason for hiding this comment

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

LGTM assuming all comments are addressed

node/src/BaseClient.ts Outdated Show resolved Hide resolved
node/src/Transaction.ts Show resolved Hide resolved
node/src/BaseClient.ts Outdated Show resolved Hide resolved
jonathanl-bq and others added 2 commits August 19, 2024 11:16
Co-authored-by: Yury-Fridlyand <[email protected]>
Signed-off-by: jonathanl-bq <[email protected]>
@jonathanl-bq jonathanl-bq merged commit ddb5af9 into valkey-io:main Aug 19, 2024
8 checks passed
@jonathanl-bq jonathanl-bq deleted the node/integ_cyip10_zunionstore branch August 19, 2024 18:46
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.

4 participants