Skip to content

Added GET argument to SET command#1412

Merged
chayim merged 5 commits intoredis:masterfrom
jiekun:2014BDuck/1411-set_command_argument
Aug 8, 2021
Merged

Added GET argument to SET command#1412
chayim merged 5 commits intoredis:masterfrom
jiekun:2014BDuck/1411-set_command_argument

Conversation

@jiekun
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

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

@chayim
Copy link
Copy Markdown
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 <[email protected]>
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