-
Notifications
You must be signed in to change notification settings - Fork 68
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
Python: adds JSON.ARRPOP command #2407
Conversation
b378399
to
a52ebf5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add changelog please
For JSONPath (`path` starts with `$`): | ||
Returns a list of bytes string replies for every possible path, representing the popped JSON values, | ||
or None for JSON values matching the path that are not an array or are an empty array. | ||
If a value is not an array, its corresponding return value is null. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If a value is not an array, its corresponding return value is null. |
Signed-off-by: Shoham Elias <shohame@amazon.com>
Signed-off-by: Shoham Elias <shohame@amazon.com>
a52ebf5
to
e10015a
Compare
Signed-off-by: Shoham Elias <116083498+shohamazon@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please address the comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CI is red
>>> await json.arrpop(client, "doc", JsonArrPopOptions(path="$.a", index=1)) | ||
[b'2'] # Pop second element from array at path $.a | ||
>>> await json.arrpop(client, "doc", JsonArrPopOptions(path="$..a")) | ||
[b'true', b'5', None] # Pop last elements from all arrays matching path `..a` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[b'true', b'5', None] # Pop last elements from all arrays matching path `..a` | |
[b'true', b'5', None] # Pop last elements from all arrays matching path `$..a` |
b"1" # First match popped (from array at path ..a) | ||
|
||
#### Even though only one value is returned from `..a`, subsequent arrays are also affected | ||
>>> await json.get(client, "doc", "$..a") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
>>> await json.get(client, "doc", "$..a") | |
>>> await json.get(client, "doc", "..a") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No I wanted to show that even if we used ..a and just one value was returned, the rest of the matching paths were popped as well
glide_client, key, JsonArrPopOptions(path="non_existing_path") | ||
) | ||
|
||
with pytest.raises(RequestError): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should make sure to include more comments for tests. it's not always obvious to readers what you're doing here.
Signed-off-by: Shoham Elias <shohame@amazon.com>
This Pull Request is linked to issue (URL): #645
Checklist
Before submitting the PR make sure the following are checked: