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 the set functions from issue #597 #743

Merged
merged 2 commits into from
Jun 16, 2024

Conversation

rootart
Copy link
Contributor

@rootart rootart commented Jun 10, 2024

@WisdomPill I took over some work that was already done here #654. Will appreciate your support in reviewing it.

@rootart rootart force-pushed the issue/597 branch 2 times, most recently from 988701b to 1a8870f Compare June 14, 2024 17:44
Copy link

codecov bot commented Jun 14, 2024

Codecov Report

Attention: Patch coverage is 73.54988% with 114 lines in your changes missing coverage. Please review.

Project coverage is 63.3%. Comparing base (294dcd0) to head (f34935c).
Report is 11 commits behind head on master.

Files Patch % Lines
tests/test_backend.py 11.0% 106 Missing ⚠️
django_redis/client/sharded.py 92.6% 3 Missing and 4 partials ⚠️
django_redis/client/default.py 99.5% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##           master    #743     +/-   ##
========================================
+ Coverage    61.5%   63.3%   +1.8%     
========================================
  Files          43      43             
  Lines        2786    3215    +429     
  Branches      161     244     +83     
========================================
+ Hits         1713    2034    +321     
- Misses       1060    1164    +104     
- Partials       13      17      +4     
Flag Coverage Δ
mypy 38.1% <36.8%> (-<0.1%) ⬇️
tests 90.3% <95.6%> (+1.0%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@WisdomPill WisdomPill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Terrific job!

I just have a couple of questions on typing, but apart from that this PR looks great.

P.S. sorry for stepping on your foot for pre-commit and ruff, I opted to merge the pre-commit autoupdate instead of different PRs

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when returning a Set can we have the container type as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I somehow missed that. I don't see a clear way how we can use generics or have better type support rather than Set[Any] for the results. Do you have other suggestions?

django_redis/client/default.py Show resolved Hide resolved
Comment on lines 948 to 952
if result is None:
return None
if isinstance(result, list):
return {self.decode(value) for value in result}
return self.decode(result)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could we have an utility method for this?

django_redis/client/sharded.py Outdated Show resolved Hide resolved
@WisdomPill WisdomPill merged commit e11150a into jazzband:master Jun 16, 2024
17 of 18 checks passed
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