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 GET argument to SET command #1412

Merged
merged 5 commits into from
Aug 8, 2021
Merged

Added GET argument to SET command #1412

merged 5 commits into from
Aug 8, 2021

Conversation

jiekun
Copy link
Contributor

@jiekun jiekun commented Oct 26, 2020

Pull Request check-list

Please make sure to review and check all of these items:

  • Does $ tox pass with this change (including linting)?
  • Does travis tests pass with this change (enable it first in your forked repo and wait for the travis build to finish)?
  • Is the new or changed code fully tested?
  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?

NOTE: these things are not required to open a PR and can be done
afterwards / while the PR is open.

Description of change

Issue: #1411

Added GET argument to SET command, new in Redis 6.2: https://redis.io/commands/set

@jiekun
Copy link
Contributor Author

jiekun commented Oct 26, 2020

To handle this, the read_response() method in connection.py may need to change a little bit.
When redis return '+OK' or '$2' (and a "OK" string value), they will be parsed into b'OK' in read_response().

What do you think @andymccurdy

@andymccurdy
Copy link
Contributor

This looks good. I'd like to wait to merge until Redis 6.2 is released so that we can include it in the unit tests.

@jiekun
Copy link
Contributor Author

jiekun commented Oct 26, 2020

Hmm but it's not working now. There is no way to tell the difference between 'OK' (bool value) and 'OK' (string value, return when ''get'' param is set to True) in client.py.

r.set(1,'OK')     # we got b'OK' response
r.set(1,'foo',get=True)  # we got b'OK' again and will be parsed as bool value

this must be solve if we want to add 'get' argument to the set command. Will take a look tomorrow

@andymccurdy
Copy link
Contributor

This should be pretty easy to solve.

def set(self, ..., get=False):
    options = {}
    ...
    if get:
        pieces.append(b'GET')
        options['get'] = True

    return self.execute_command(*pieces, **options)

Then we make a response callback for SET:

def parse_set(response, **options):
    if options.get('get'):
        # return the string
    # return the boolean

@jiekun
Copy link
Contributor Author

jiekun commented Oct 26, 2020

haha allright. I will deal it in this way. I thought it should be more concise before, without passing any extra argument.

will catch it up tomorrow.

@jiekun jiekun changed the title (WIP) Added GET argument to SET command Added GET argument to SET command Oct 28, 2020
@andymccurdy
Copy link
Contributor

This looks good. Thanks! Tagging this as Redis 6.2 and will merge once the server is released.

@chayim
Copy link
Contributor

chayim commented Aug 8, 2021

Thank you for your contribution @2014BDuck. I just merged, and validated with redis server 6.2.5. Approving, and merging into master.

@chayim chayim merged commit 2789f08 into redis:master Aug 8, 2021
Andrew-Chen-Wang added a commit to aio-libs-abandoned/aioredis-py that referenced this pull request Oct 8, 2021
Added GET argument to SET command (redis/redis-py#1412)

Signed-off-by: Andrew-Chen-Wang <acwangpython@gmail.com>
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.

3 participants