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

Add support for SMISMEMBER command added in redis 6.2.0 #1486

Closed
wants to merge 2 commits into from
Closed

Add support for SMISMEMBER command added in redis 6.2.0 #1486

wants to merge 2 commits into from

Conversation

ported-pw
Copy link

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

  • Add support for the SMISMEMBER command added in Redis 6.2.0 as mentioned in Redis 6.2 New Commands Check List #1434.
  • Bump the server version used in the Docker test environment to 6.2.0 to test the new command
  • Add support for the channels (new in 6.2.0) setting in acl_getuser and fix ACL tests using a version check to make them pass on 6.2.0. This does NOT add support for the channel ACLs in acl_setuser or support for it in general.

@ported-pw
Copy link
Author

ported-pw commented May 25, 2021

Some open questions:

  • Is it actually a good idea to fix things related to 6.2.0 in this PR to make tests pass or should the tests rather fail so this PR only includes the commit for the SMISMEMBER command?
  • Should I actually convert the returned integers to booleans or leave them as integers, more closely matching the redis documentation? What is the philosophy here, how much should we change about the returned data?
  • Is it a good idea to use string_keys_to_dict even though it only adds one command? The thought here was that an Array of "Boolean" replies would be something that could be used elsewhere too.

Also, the CI failed because of
toomanyrequests: You have reached your pull rate limit. You may increase the limit by authenticating and upgrading: https://www.docker.com/increase-rate-limit

@ported-pw
Copy link
Author

Like mentioned in #1460 (comment), an unrelated unit test is failing on Redis 6.2.3.

@chayim chayim self-requested a review August 8, 2021 09:27
@chayim chayim mentioned this pull request Aug 19, 2021
66 tasks
@chayim
Copy link
Contributor

chayim commented Sep 30, 2021

Like mentioned in #1460 (comment), an unrelated unit test is failing on Redis 6.2.3.

IMHO I think this PR should just focus on the command being implemented. I'm a big fan of not bundling unrelated changes. Any chance you'd like to merge master in, and we can possibly get this in for the upcoming release? WDYT?

@chayim chayim mentioned this pull request Nov 3, 2021
@chayim
Copy link
Contributor

chayim commented Nov 3, 2021

SMISMEMBER support itself is superseded by #1667 which separates out just the initial change for the command itslef.

@chayim chayim closed this Nov 3, 2021
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.

2 participants