SET IFEQ command implemented in node + tests #2909
Conversation
Signed-off-by: Maayan Shani <[email protected]> Signed-off-by: Maayan Shani <[email protected]>
Signed-off-by: Maayan Shani <[email protected]> Signed-off-by: Maayan Shani <[email protected]>
Maayanshani25
force-pushed
the
maayans-ifeq
branch
from
3ac0f80 to
a7300c5
Compare
Signed-off-by: Maayan Shani <[email protected]>
Signed-off-by: Maayan Shani <[email protected]>
Maayanshani25
force-pushed
the
maayans-ifeq
branch
from
041a662 to
febef71
Compare
Signed-off-by: Maayan Shani <[email protected]>
Signed-off-by: Maayan Shani <[email protected]>
Maayanshani25
force-pushed
the
maayans-ifeq
branch
from
04c956e to
3acc0d9
Compare
node/src/Commands.ts
Outdated
| | { | ||
| /** | ||
| * `onlyIfDoesNotExist` - Only set the key if it does not already exist. | ||
| * Equivalent to `NX` in the Valkey API. |
There was a problem hiding this comment.
I don't like the usage of Equivalent to like we are creating something separate which has some interaction.
Let's find better ways to express it.
Maybe ֿsimple as:
* `onlyIfDoesNotExist` - Only set the key if it does not already exist.
* `NX` in the Valkey API.
Relevant to all the below options. I'm aware that it is not you who decided to use it, but still, I think it needs to be changed.
There was a problem hiding this comment.
exists a lot of times. you want me to change all? (other commands also)
node/src/BaseClient.ts
Outdated
| * | ||
| * // Example usage of set method with conditional option IFEQ | ||
| * const result5 = await client.set("key", "ifeq_value", {conditionalSet: "onlyIfEqual", comparisonValue: "new_value"); | ||
| * console.log(result5); // Output: 'OK' - Set "ifeq_value" to "key" only if comparisonValue is equal to the value of "key". |
There was a problem hiding this comment.
nit: value of "key" is confusing a bit.
Maybe the current value set to key?
Think about it.
There was a problem hiding this comment.
But maybe the value haven't set yet so it will be null. For me it's clear enough but i will think of a better one
There was a problem hiding this comment.
Its not so important, but if you find something it will be great
| * `onlyIfEqual` - Only set the key if the comparison value equals the key value. | ||
| * Equivalent to `IFEQ` in the Valkey API. | ||
| */ | ||
| conditionalSet: "onlyIfEqual"; |
There was a problem hiding this comment.
IFEQ is short for "if equal", why do we need the "only"?
It is not aligned with the valkey naming, and I don't think it's to clarify something which is not clear.
There was a problem hiding this comment.
it's like the other conditions onlyIfExists == NX and onlyIfDoesNotExist == EX. That is why i used only as a prefix
Signed-off-by: Maayan Shani <[email protected]>
Issue link
This Pull Request is linked to issue (URL): [/issues/2811]
Description
This pull Request is implemented the
IFEQoption in theSETcommamnd. Tests were added to the shared tests, and docstring to the command and the related methods and structures.Checklist
Before submitting the PR make sure the following are checked: