Skip to content

Conversation

@oliverhaas
Copy link

@oliverhaas oliverhaas commented Nov 9, 2025

See #797.

I can always clean this up or change the structure, but this is how it made sense to me right now.

I can also add more commands to this PR or in a next PR for sorted sets, but these are the main ones I would say, and I'm trying to not blow this up too much.

If there is nothing majorly wrong with this, I'd like to add like another 2-3 PRs worth of commands soon, since I'm using Redis quite heavily right now and I'd rather not have too many custom code in my projects which should probably just go into django-redis. From my point of view, anyway.

@WisdomPill
Copy link
Member

looks good!

but this change of structure will need to become consistent with the other kind of commands too, are you willing to make PRs to separate the rest of the commands?

I would say, hash and set also need their own mixins...

Last but not least, how do you think could bring benefits for going async?

@codecov
Copy link

codecov bot commented Nov 9, 2025

Codecov Report

❌ Patch coverage is 35.53922% with 263 lines in your changes missing coverage. Please review.
✅ Project coverage is 38.2%. Comparing base (7043888) to head (740493b).

Files with missing lines Patch % Lines
tests/test_backend_sorted_sets.py 1.3% 155 Missing ⚠️
django_redis/client/mixins/sorted_sets.py 59.5% 22 Missing and 55 partials ⚠️
django_redis/cache.py 33.4% 28 Missing ⚠️
django_redis/client/mixins/protocols.py 78.6% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##           master    #798     +/-   ##
========================================
- Coverage    38.5%   38.2%   -0.3%     
========================================
  Files          44      48      +4     
  Lines        3320    3727    +407     
  Branches      245     301     +56     
========================================
+ Hits         1276    1420    +144     
- Misses       1799    2006    +207     
- Partials      245     301     +56     
Flag Coverage Δ
mypy 38.2% <35.6%> (-0.3%) ⬇️

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.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@oliverhaas
Copy link
Author

oliverhaas commented Nov 9, 2025

looks good!

but this change of structure will need to become consistent with the other kind of commands too, are you willing to make PRs to separate the rest of the commands?

I would say, hash and set also need their own mixins...

Last but not least, how do you think could bring benefits for going async?

What I would personally would aim for in the near future is:

  • Add most important list commands (as mixin)
  • Add more hash commands (+ refactor to mixin)
  • Refactor set commands to mixin (don't think anything major is missing here...?)
  • Add some more general redis commands like .type() and .info(). I personally would like to have these, but they are definitely a bit more awkward to have in a more general sense. But as long es these commands will work with redis and valkey in the foreseeable future, it's probably okay. Also, nobody has to use them if they do not want to 😄

If that sounds good I'm hoping to continue basically today. Except for the last point there is really not much to it. You could review after you've seen the whole package of those 4-5 PRs, so you know I'm not quitting half way. As long as it's realistic from your end these will make it into django-redis without anything big on top of what I've laid out, I'm happy to continue here.

Regarding async: I personally don't have much use for async unfortunately, just because my current employer went with sync, although I would really like to use it more. It's not clear to me what the goal and approach would be for django-redis. If you have some github issue or something I'm not seeing let me know.

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