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

Made sync lock consistent and added types to it #2137

Merged
merged 4 commits into from
Jun 1, 2022

Conversation

WisdomPill
Copy link
Contributor

@WisdomPill WisdomPill commented Apr 24, 2022

Fixes: #1348
Needed for: jazzband/django-redis#603

Pull Request check-list

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

  • Does $ tox pass with this change (including linting)?
  • Do the CI tests pass with this change (enable it first in your forked repo and wait for the github action 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)?
  • Was the change added to CHANGES file?

Description of change

Added blocking flag to client.lock and types to sync lock.
Some more nit picking inconsistency was fixed so that regardless on who the lock is used, all the options are the same.

If this is okay I will also add the same to the async version of the lock

@codecov-commenter
Copy link

codecov-commenter commented Apr 24, 2022

Codecov Report

Merging #2137 (80c47b6) into master (e5e265d) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #2137   +/-   ##
=======================================
  Coverage   92.44%   92.44%           
=======================================
  Files         104      104           
  Lines       24364    24384   +20     
=======================================
+ Hits        22524    22543   +19     
- Misses       1840     1841    +1     
Impacted Files Coverage Δ
redis/client.py 89.46% <ø> (ø)
redis/cluster.py 91.82% <ø> (ø)
redis/lock.py 100.00% <100.00%> (ø)
redis/typing.py 90.62% <100.00%> (+0.30%) ⬆️
tests/test_lock.py 100.00% <100.00%> (ø)
tests/test_cluster.py 97.48% <0.00%> (-0.12%) ⬇️
redis/asyncio/connection.py 84.72% <0.00%> (+0.11%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e5e265d...80c47b6. Read the comment docs.

@WisdomPill
Copy link
Contributor Author

@chayim or @dvora-h friendly ping

@dvora-h
Copy link
Collaborator

dvora-h commented May 4, 2022

Thanks @WisdomPill! It looks great to me!

As you suggested - I would really appreciate if you add an async version as well

@WisdomPill
Copy link
Contributor Author

nice! probably in the weekend I will be back at it :)

@WisdomPill
Copy link
Contributor Author

sorry I did not finish working on the async version yet... I just started and got caught up by other things

@dvora-h
Copy link
Collaborator

dvora-h commented Jun 1, 2022

@WisdomPill I want to merge this PR so please open another PR when you done with the async version, and feel free to let me know if you need help with it

@WisdomPill
Copy link
Contributor Author

👍

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.

Question: why is blocking missing as a lock() parameter?
3 participants