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 a count parameter to lpop/rpop for redis >= 6.2.0 #1487

Merged
merged 9 commits into from
Aug 5, 2021
Merged

Add a count parameter to lpop/rpop for redis >= 6.2.0 #1487

merged 9 commits into from
Aug 5, 2021

Conversation

wavenator
Copy link
Contributor

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

Please provide a description of the change here.

@wavenator wavenator changed the title Added count parameter to lpop/rpop Add a count parameter to lpop/rpop that has been added since version 6.2.0 May 27, 2021
@chayim
Copy link
Contributor

chayim commented Jul 22, 2021

@wavenator Mind updating your branch with the latest master. CI has moved to GitHub actions, and the unit test were fixed. Happy to review when ready.

@wavenator
Copy link
Contributor Author

@chayim Thanks for your reply. My tests have been fixed. One more test failed that wasn't mine. I appreciate your help with this. By the way, I hope you squash all my commits.

@chayim chayim self-requested a review July 26, 2021 15:06
@chayim
Copy link
Contributor

chayim commented Jul 26, 2021

@wavenator Looks great, and checks out against the lpop and rpop docs. I think the unit test failed because the client kill run (my change a while back!) didn't properly turf all connections. I'm going to play with this a bit more, to be sure - and re-run the action accordingly.

@wavenator
Copy link
Contributor Author

@wavenator Looks great, and checks out against the lpop and rpop docs. I think the unit test failed because the client kill run (my change a while back!) didn't properly turf all connections. I'm going to play with this a bit more, to be sure - and re-run the action accordingly.

I will be more than happy to help you if you need it!

@chayim
Copy link
Contributor

chayim commented Aug 5, 2021

@wavenator I have a PR up, purely to update to the latest redis-docker (and fix client kill), which will hence pass yours. But, I uncovered a flaky test (this same one!) as my reminder. I've asked @AvitalFineRedis to fix the flaky test from her PR, so that I don't blindly merge my change.

redis-py had a period of time where unit tests in master were failing, so my goal is to keep those, always passing - as first order.

@chayim
Copy link
Contributor

chayim commented Aug 5, 2021

Validated with latest master, tests pass.

@wavenator Thanks a tonne for your contribution, and patience in the back-and-forth. Much obliged!

@chayim chayim changed the title Add a count parameter to lpop/rpop that has been added since version 6.2.0 Add a count parameter to lpop/rpop for redis >= 6.2.0 Aug 5, 2021
@chayim chayim merged commit 238f69e into redis:master Aug 5, 2021
Andrew-Chen-Wang added a commit to aio-libs-abandoned/aioredis-py that referenced this pull request Oct 8, 2021
Add a count parameter to lpop/rpop for redis >= 6.2.0 (redis/redis-py#1487)

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.

2 participants