Add the getdel command available from redis 6.2.0 #1460
Conversation
|
OK, I have been lazy: I deactivated the unit test that failed. Travis is OK, but of course Codecov is not happy. The thing is that I am not sure about this test_pubsub_worker_thread_exception_handler unit test. I will try and understand, but if you have some information that could help, I am really interested. |
| @@ -10,7 +10,7 @@ | |||
| # redis 6 release candidates report a version number of 5.9.x. Use this | |||
| # constant for skip_if decorators as a placeholder until 6.0.0 is officially | |||
| # released | |||
| REDIS_6_VERSION = '5.9.0' | |||
| REDIS_6_VERSION = '6.2.0' | |||
There was a problem hiding this comment.
This is not correct. REDIS_6_VERSION actually means "Since which version Redis6 feature introduced". Please check the following usage.
@skip_if_server_version_lt(REDIS_6_VERSION)
So you cannot change it to 6.2.0
There was a problem hiding this comment.
You are right, this is a poor work around. The issue is that I do not understand why the tests are failing with Redis 6.2. All I want here is to add the getdel verb that is now available in Redis. I would appreciate a lot some help to resolve the Unit Test issues of that PR. If not I will cancel it and wait for somebody else to add getdel in another PR.
| @@ -474,7 +474,7 @@ class TestPubSubSubcommands: | |||
| def test_pubsub_channels(self, r): | |||
| p = r.pubsub() | |||
| p.subscribe('foo', 'bar', 'baz', 'quux') | |||
| for i in range(4): | |||
| for _ in range(4): | |||
There was a problem hiding this comment.
Good catch! But personally speaking, I don't prefer irrelevant changes being included in this PR. What do you think @andymccurdy
There was a problem hiding this comment.
I am very new contributing in open source project. So please forgive my silly question. Are you saying here that resolving this Pylint warning should not be done in this PR because it is not its main focus? I would have thought we should use every opportunity to reduce the technical debt, especially if it is clear that it will not break anything.
|
@pcart-grandjean Thank you for the contribution! There's some good news / bad news. The good news is that we have getdel support in master. But the bad news is that I hadn't seen this PR when that happened. My apologies. I'm going to close this PR as a result - and again, my apologies. On the other hand, the smaller changes you were discussing earlier with @2014BDuck like this one we'd love to see in redis-py. Anything that improves quality is always welcome! |
|
Hello I am very interested to use the |
|
@pcart-grandjean A new version of redis-py is coming in the near future! However, until then, will doing a pip install of master at a hash suffice?
|
Pull Request check-list
Please make sure to review and check all of these items:
$ toxpass with this change (including linting)?NOTE: these things are not required to open a PR and can be done
afterwards / while the PR is open.
Description of change
Coming with Redis 6.2 is the new command GETDEL. This pull request allows to also have it in this Python interface.