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

added hincrBy and hincrByFloat in node #502

Merged
merged 1 commit into from
Oct 17, 2023
Merged

added hincrBy and hincrByFloat in node #502

merged 1 commit into from
Oct 17, 2023

Conversation

adanWattad
Copy link
Contributor

Description of changes:
added hincrBy and hincrByFloat in node

@adanWattad adanWattad requested a review from barshaul October 10, 2023 10:37
@barshaul barshaul mentioned this pull request Oct 11, 2023
@@ -447,6 +451,44 @@ export class BaseClient {
return this.createWritePromise(createHMGet(key, fields));
}

/** Increments the number stored at field in the hash stored at key by increment.
Copy link
Collaborator

Choose a reason for hiding this comment

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

field => `field`
key => `key`
by increment => by `amount`.

*
* @param key - The key of the hash.
* @param amount - The amount to increment.
* @param field - The field in the hash stored at key to increment it's value.
Copy link
Collaborator

Choose a reason for hiding this comment

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

it's => its

* @param key - The key of the hash.
* @param amount - The amount to increment.
* @param field - The field in the hash stored at key to increment it's value.
* @returns the value of the field in the hash stored at key after the increment, An error is returned if the key contains a value
Copy link
Collaborator

Choose a reason for hiding this comment

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

the value of the field in the hash stored at key after the increment.
An error will be returned if the key holds a value of an incorrect type (not a string) or if it contains a string that cannot be represented as an integer.

}

/** Increment the string representing a floating point number stored at field in the hash stored at key by increment.
* By using a negative increment value, the result is that the value stored at field in the hash stored at key is decremented.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it relevant also for hincrBy?

return this.createWritePromise(createHIncrBy(key, field, amount));
}

/** Increment the string representing a floating point number stored at field in the hash stored at key by increment.
Copy link
Collaborator

Choose a reason for hiding this comment

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

same for marking the variables with ``

@@ -312,6 +316,38 @@ export class BaseTransaction {
this.commands.push(createHMGet(key, fields));
}

/** Increments the number stored at field in the hash stored at key by increment.
Copy link
Collaborator

Choose a reason for hiding this comment

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

same for all comments from BaseClient

key: string,
field: string,
amount: number
): Promise<string> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

why incrbyfloat returns a string and incrby returns number? I would expect both to return a number

Copy link
Collaborator

Choose a reason for hiding this comment

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

After we talked about it: It's because RESP2 doesn't support double/float numbers so it returns as a string.
lets leave it as a string for now, and add the command to this issue:
#504
change the headline to
Change return type of several commands for types introduced in RESP3
and edit the content so it would have a table of the command, its current return type, and the required type to change to after we'll have support in RESP3

};
expect(await client.hset(key, fieldValueMap)).toEqual(1);
expect(await client.hincrBy(key, field, 1)).toEqual(11);
expect(await client.hget(key, field)).toEqual("11");
Copy link
Collaborator

Choose a reason for hiding this comment

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

we don't need to check it also with hget, it is enough to verify that the return result of hincrBy is the expected result. Please fix in all places

await runTest(async (client: BaseClient) => {
const key1 = uuidv4();
const key2 = uuidv4();
const nonExistingKey1 = uuidv4();
Copy link
Collaborator

Choose a reason for hiding this comment

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

nonExistingKey1 -> can be changed to nonExistingKey, if there's no nonExistingKey2, right?

);

it(
"hincrBy and hincrByFloat with a field that contains a value of string that can not be represented as integer",
Copy link
Collaborator

Choose a reason for hiding this comment

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

as integer or float

@adanWattad
Copy link
Contributor Author

@barshaul ready for another round.

@adanWattad adanWattad requested a review from barshaul October 11, 2023 10:56
@adanWattad adanWattad merged commit 72f7564 into main Oct 17, 2023
6 checks passed
@adanWattad adanWattad deleted the node/hincr branch October 17, 2023 12:32
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.

2 participants