Implemented IFEQ set command in python with tests #2962

Angraybill · 2025-01-16T19:09:03Z

Implements the IFEQ option into the SET command. Tests were added on test_async_client.py.

Issue link

This Pull Request is linked to issue (URL): #2811

Checklist

Before submitting the PR make sure the following are checked:

This Pull Request is related to one issue.
Commit message has a detailed description of what changed and why.
Tests are added or updated.
CHANGELOG.md and documentation files are updated.
Destination branch is correct - main or release
Create merge commit if merging release branch into main, squash otherwise.

Angraybill · 2025-01-16T19:18:29Z

I'm open to suggestions, critique, and potential variable name changes.

python/python/glide/async_commands/core.py

avifenesh · 2025-01-17T13:57:47Z

python/python/glide/async_commands/core.py

@@ -73,10 +73,12 @@ class ConditionalChange(Enum):
    A condition to the `SET`, `ZADD` and `GEOADD` commands.
    - ONLY_IF_EXISTS - Only update key / elements that already exist. Equivalent to `XX` in the Valkey API.
    - ONLY_IF_DOES_NOT_EXIST - Only set key / add elements that does not already exist. Equivalent to `NX` in the Valkey API.
+    - ONLY_IF_EQUAL - Only update key if the provided value is equal to the old value. Equivalent to `IFEQ` in the Valkey API.


It is a bit of a problem, since this type is optional in ZADD and GEOADD as well, and we can't let IFEQ part of it.
We need to plan it a bit different, but I want to make sure first if there are any plans to add it to the other in the future so we can take a smart decision of how to handle it.
Let's keep working on the PR, but keep in mind that we are going to make changes here, it will need to be a separate type. Ill tag on the discussion in Valkey WS, and we will discuss it offline to understand it better.

In addition, while the others are not requiring extra parameter, IFEQ, and we do not want to allow users mistakenly adding one of the parameters and the other and fail on run time, and we want to enforce to the best we can do, the right usage of the type, more like we do in the ExpiryType than as we do it in ConditionalChange, but let's wait for response from community, and then lets plan it better.

To whom come across - the solution we decided to go on is a Union between ConditionalChange and a new class OnlyIfEqual which will include comparison_value as a field.

The reason not to create a separate SetConditionalChange Enum to use a Union between the two, is that Enum doesn't enforce using comparison_value along with OnlyIfEqual and will end up as a runtime error, while can be avoided with type checking when it is a class.
The reason that the class is OnlyIfEqual and not more generic is because to make it more generic we need to avoid setting specific required fields, and that's miss the logic in trying to enforce comparison_value.

python/python/glide/async_commands/core.py

python/python/tests/test_async_client.py

Angraybill · 2025-01-17T18:38:20Z

It would appear that I added one commit too many to the PR. It won't change anything (it was just my typo fixes from my last PR) but it's a little redundant.

python/python/tests/test_async_client.py

python/python/glide/async_commands/core.py

avifenesh · 2025-01-17T23:33:25Z

python/python/glide/async_commands/core.py

+
+        elif isinstance(conditional_set, OnlyIfEqual):
+            args.append("IFEQ")
+            if conditional_set.comparison_value:


Since we added the parameter to be a Class with a field this if is not relevant anymore, the class has to have this field.

avifenesh · 2025-01-17T23:40:11Z

python/python/glide/async_commands/core.py

+@dataclass
+class OnlyIfEqual():
+    """
+    Condition to the `SET` command


Suggested change

Condition to the `SET` command

Change condition to the `SET` command,

For additional conditonal options see ConditionalChange

avifenesh · 2025-01-17T23:44:18Z

Test fails since you created a new type, hence needs to be imported to https://github.com/valkey-io/valkey-glide/blob/main/python/python/glide/__init__.py to be available for users.

avifenesh · 2025-01-17T23:45:13Z

Please fix linting failures

python/python/glide/async_commands/core.py

Angraybill · 2025-01-20T06:53:02Z

The formatter reformatted the way I call the SET command to a way that isn't consistent with the rest of the test files.

avifenesh · 2025-01-20T15:30:37Z

The formatter reformatted the way I call the SET command to a way that isn't consistent with the rest of the test files.

Please give more details

Angraybill · 2025-01-20T17:18:47Z

All of the times SET is called, they format it like
glide_client.set( "key", "value", )
But the formatter redid it so it looks like
glide_client.set( "key" "value )

Angraybill · 2025-01-20T17:19:27Z

ok GitHub did not format that the way I hoped. Basically the formatter put new lines between every parameter despite that not happening in the rest of the test cases

…for IFEQ Signed-off-by: Angraybill <[email protected]>

Signed-off-by: Angraybill <[email protected]>

avifenesh

Great work! Even after the surprising challenges, it looks good, clean code, good docs, and extra fixes. Approved.

* Define new function, change paramter type to Union, add conditionals for IFEQ Signed-off-by: Angraybill <[email protected]> * Tests for IFEQ, positive and negative cases Signed-off-by: Angraybill <[email protected]> * Update Changelog Signed-off-by: Angraybill <[email protected]> * Import OnlyIfEqual in init file Signed-off-by: Angraybill <[email protected]> --------- Signed-off-by: Angraybill <[email protected]>

Angraybill requested a review from a team as a code owner January 16, 2025 19:09

Yury-Fridlyand added the python Python wrapper label Jan 16, 2025

avifenesh reviewed Jan 17, 2025

View reviewed changes

python/python/glide/async_commands/core.py Show resolved Hide resolved

avifenesh reviewed Jan 17, 2025

View reviewed changes

python/python/glide/async_commands/core.py Outdated Show resolved Hide resolved

avifenesh reviewed Jan 17, 2025

View reviewed changes

python/python/glide/async_commands/core.py Outdated Show resolved Hide resolved

avifenesh reviewed Jan 17, 2025

View reviewed changes

python/python/tests/test_async_client.py Show resolved Hide resolved

avifenesh added the Feature Additional feature, big or small label Jan 17, 2025

avifenesh added this to the 1.3 milestone Jan 17, 2025

Angraybill force-pushed the angraybill_ifeq branch from f1ae208 to 9250c5f Compare January 17, 2025 18:04

avifenesh reviewed Jan 17, 2025

View reviewed changes

python/python/tests/test_async_client.py Show resolved Hide resolved

avifenesh reviewed Jan 17, 2025

View reviewed changes

python/python/glide/async_commands/core.py Show resolved Hide resolved

avifenesh reviewed Jan 17, 2025

View reviewed changes

python/python/glide/async_commands/core.py Outdated Show resolved Hide resolved

Angraybill force-pushed the angraybill_ifeq branch from 63f6f7d to 87520fe Compare January 20, 2025 19:19

avifenesh removed this from the 1.3 milestone Jan 21, 2025

Angraybill added 4 commits January 21, 2025 12:28

Define new function, change paramter type to Union, add conditionals …

7f8f4dd

…for IFEQ Signed-off-by: Angraybill <[email protected]>

Tests for IFEQ, positive and negative cases

34520da

Signed-off-by: Angraybill <[email protected]>

Update Changelog

a9eb9bf

Signed-off-by: Angraybill <[email protected]>

Import OnlyIfEqual in init file

b223110

Signed-off-by: Angraybill <[email protected]>

Angraybill force-pushed the angraybill_ifeq branch from b7bd358 to b223110 Compare January 21, 2025 20:32

avifenesh assigned Angraybill Jan 21, 2025

avifenesh approved these changes Jan 21, 2025

View reviewed changes

Angraybill merged commit e29f6d3 into valkey-io:main Jan 21, 2025
22 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implemented IFEQ set command in python with tests #2962

Implemented IFEQ set command in python with tests #2962

Angraybill commented Jan 16, 2025 •

edited

Loading

Angraybill commented Jan 16, 2025

avifenesh Jan 17, 2025

avifenesh Jan 17, 2025

Angraybill commented Jan 17, 2025

avifenesh Jan 17, 2025

avifenesh Jan 17, 2025

avifenesh commented Jan 17, 2025

avifenesh commented Jan 17, 2025

Angraybill commented Jan 20, 2025

avifenesh commented Jan 20, 2025

Angraybill commented Jan 20, 2025

Angraybill commented Jan 20, 2025

avifenesh left a comment

	Condition to the `SET` command
	Change condition to the `SET` command,
	For additional conditonal options see ConditionalChange

Implemented IFEQ set command in python with tests #2962

Implemented IFEQ set command in python with tests #2962

Conversation

Angraybill commented Jan 16, 2025 • edited Loading

Issue link

Checklist

Angraybill commented Jan 16, 2025

avifenesh Jan 17, 2025

Choose a reason for hiding this comment

avifenesh Jan 17, 2025

Choose a reason for hiding this comment

Angraybill commented Jan 17, 2025

avifenesh Jan 17, 2025

Choose a reason for hiding this comment

avifenesh Jan 17, 2025

Choose a reason for hiding this comment

avifenesh commented Jan 17, 2025

avifenesh commented Jan 17, 2025

Angraybill commented Jan 20, 2025

avifenesh commented Jan 20, 2025

Angraybill commented Jan 20, 2025

Angraybill commented Jan 20, 2025

avifenesh left a comment

Choose a reason for hiding this comment

Angraybill commented Jan 16, 2025 •

edited

Loading