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

Python: adds JSON.STRLEN, JSON.STRAPPEND commands #2372

Merged
merged 4 commits into from
Oct 27, 2024

Conversation

shohamazon
Copy link
Collaborator

@shohamazon shohamazon commented Sep 30, 2024

replaced #1187

For more information about the returned type, see `TJsonResponse`.

Examples:
>>> from glide import json as redisJson
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
>>> from glide import json as redisJson
>>> from glide import json

remove or rename alias everywhere

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah you're right

"""
Appends the specified `value` to the string stored at the specified `path` within the JSON document stored at `key`.

See https://redis.io/commands/json.strapped/ for more details.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
See https://redis.io/commands/json.strapped/ for more details.
See https://redis.io/commands/json.strappend/ for more details.

client (TRedisClient): The Redis client to execute the command.
key (TEncodable): The key of the JSON document.
value (TEncodable): The value to append to the string. Must be wrapped with single quotes. For example, to append "foo", pass '"foo"'.
path (Optional[TEncodable]): The JSONPath to specify. Default is root `$`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
path (Optional[TEncodable]): The JSONPath to specify. Default is root `$`.
path (Optional[TEncodable]): Represents the path within the JSON document where the string value will be appended. Default is root `$`.

To be consistent. Although, this description is much too verbose. I'd prefer that we go with something concise that mirrors the command description, like:

Path within the JSON document.  If not specified, the root (`$`) is used. 

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed all of them, added Default to None. since the documentation say its $ but its actually . so I better not specify whats exactly it is


Examples:
>>> from glide import json as redisJson
>>> import json
Copy link
Contributor

Choose a reason for hiding this comment

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

you also have json here, and you use json.dump.

python/python/glide/async_commands/server_modules/json.py Outdated Show resolved Hide resolved
>>> from glide import json as redisJson
>>> import json
>>> from glide import json
>>> import json as jsonpy
Copy link
Contributor

Choose a reason for hiding this comment

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

I wouldn't recommend aliasing standard libraries. Users won't want to refactor their code for our client.

Copy link
Collaborator Author

@shohamazon shohamazon Oct 27, 2024

Choose a reason for hiding this comment

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

I will change it once I change the module name

return cast(
TJsonResponse[Optional[int]],
await client.custom_command(
["JSON.STRLEN", key, path] if path else ["JSON.STRLEN", key]
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: personally I don't like these shortcut forms because they're specific to python

Copy link
Contributor

@acarbonetto acarbonetto left a comment

Choose a reason for hiding this comment

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

  • Add a CHANGELOG entry

shohamazon and others added 2 commits October 27, 2024 07:59
Signed-off-by: Shoham Elias <shohame@amazon.com>
Co-authored-by: Andrew Carbonetto <andrew.carbonetto@improving.com>
Co-authored-by: jonathanl-bq <72158117+jonathanl-bq@users.noreply.github.com>
Signed-off-by: Shoham Elias <116083498+shohamazon@users.noreply.github.com>
Signed-off-by: Shoham Elias <shohame@amazon.com>
Signed-off-by: Shoham Elias <shohame@amazon.com>
@shohamazon shohamazon merged commit 8d09a1c into release-1.2 Oct 27, 2024
16 checks passed
@shohamazon shohamazon deleted the python/json.strlen branch October 27, 2024 08:33
BoazBD pushed a commit to BoazBD/valkey-glide that referenced this pull request Oct 27, 2024
---------

Signed-off-by: Shoham Elias <shohame@amazon.com>
Signed-off-by: Shoham Elias <116083498+shohamazon@users.noreply.github.com>
Co-authored-by: Andrew Carbonetto <andrew.carbonetto@improving.com>
Co-authored-by: jonathanl-bq <72158117+jonathanl-bq@users.noreply.github.com>
BoazBD pushed a commit that referenced this pull request Oct 27, 2024
---------

Signed-off-by: Shoham Elias <shohame@amazon.com>
Signed-off-by: Shoham Elias <116083498+shohamazon@users.noreply.github.com>
Co-authored-by: Andrew Carbonetto <andrew.carbonetto@improving.com>
Co-authored-by: jonathanl-bq <72158117+jonathanl-bq@users.noreply.github.com>
BoazBD pushed a commit that referenced this pull request Oct 27, 2024
---------

Signed-off-by: Shoham Elias <shohame@amazon.com>
Signed-off-by: Shoham Elias <116083498+shohamazon@users.noreply.github.com>
Co-authored-by: Andrew Carbonetto <andrew.carbonetto@improving.com>
Co-authored-by: jonathanl-bq <72158117+jonathanl-bq@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
python Python wrapper
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants