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

Add command ZRandMember #2013

Merged

Conversation

tjzhang-BQ
Copy link
Collaborator

No description provided.

@tjzhang-BQ tjzhang-BQ added the node Node.js wrapper label Jul 24, 2024
@tjzhang-BQ tjzhang-BQ requested a review from a team as a code owner July 24, 2024 23:58
@tjzhang-BQ tjzhang-BQ force-pushed the node/integ_tjz_zrandmember_valkey-88 branch 4 times, most recently from 945db32 to 42bc183 Compare July 25, 2024 00:03
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/Commands.ts Outdated Show resolved Hide resolved
node/src/Commands.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/BaseClient.ts Outdated Show resolved Hide resolved
node/src/Commands.ts Outdated Show resolved Hide resolved
node/src/Transaction.ts Show resolved Hide resolved
node/src/Transaction.ts Show resolved Hide resolved
node/src/Transaction.ts Show resolved Hide resolved
@tjzhang-BQ tjzhang-BQ force-pushed the node/integ_tjz_zrandmember_valkey-88 branch 3 times, most recently from dde58fa to ddf2c97 Compare July 25, 2024 22:49
node/src/BaseClient.ts Outdated Show resolved Hide resolved
node/src/Commands.ts Outdated Show resolved Hide resolved
node/src/Transaction.ts Show resolved Hide resolved
@Yury-Fridlyand Yury-Fridlyand marked this pull request as draft July 26, 2024 17:37
@Yury-Fridlyand

This comment was marked as resolved.

TJ Zhang added 4 commits July 26, 2024 12:59
address comments

Signed-off-by: TJ Zhang <[email protected]>
Signed-off-by: TJ Zhang <[email protected]>
Signed-off-by: TJ Zhang <[email protected]>
Signed-off-by: TJ Zhang <[email protected]>
@tjzhang-BQ tjzhang-BQ force-pushed the node/integ_tjz_zrandmember_valkey-88 branch from 408a7e0 to a8b3e71 Compare July 26, 2024 20:00
@tjzhang-BQ tjzhang-BQ marked this pull request as ready for review July 26, 2024 20:00
TJ Zhang added 2 commits July 26, 2024 13:01
Signed-off-by: TJ Zhang <[email protected]>
Copy link
Collaborator

@yipin-chen yipin-chen left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -185,6 +185,7 @@ export type ReturnType =
| null
| boolean
| bigint
| Buffer
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove?

Copy link
Collaborator Author

@tjzhang-BQ tjzhang-BQ Jul 26, 2024

Choose a reason for hiding this comment

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

This is required to make the change in transaction tests

node/src/BaseClient.ts 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
* console.log(payload2); // Output: null
* ```
*/
public async zrandmember(key: string): Promise<string | null> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

consider:

Suggested change
public async zrandmember(key: string): Promise<string | null> {
public async zrandmember(key: string, count?: number): Promise<string | null | string[]> {

Copy link
Collaborator

Choose a reason for hiding this comment

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

let's not do this. just because we can merge signatures, it's not always the right thing to do.
it would reduce the number of commands we need to maintain, but it means we have to rely on the user reading and understanding the documentation

node/src/Transaction.ts Outdated Show resolved Hide resolved
node/src/Transaction.ts Outdated Show resolved Hide resolved
node/src/Transaction.ts Outdated Show resolved Hide resolved
node/src/Transaction.ts Outdated Show resolved Hide resolved
Signed-off-by: TJ Zhang <[email protected]>
node/src/BaseClient.ts Outdated Show resolved Hide resolved
Signed-off-by: TJ Zhang <[email protected]>
@tjzhang-BQ tjzhang-BQ merged commit e3dd23d into valkey-io:main Jul 29, 2024
8 checks passed
@tjzhang-BQ tjzhang-BQ deleted the node/integ_tjz_zrandmember_valkey-88 branch July 29, 2024 22:10
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