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

Added enforce_encoding #602

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

WisdomPill
Copy link
Member

@WisdomPill WisdomPill commented Apr 18, 2022

Fixes: #595

Added enforce_encoding option for encoding even bool and int using serializer when this behavior is wanted.

@syphar for some strange reason I can't add you in the reviewers and it did not even suggest you when I was tagging you

@codecov
Copy link

codecov bot commented Apr 18, 2022

Codecov Report

Merging #602 (7c3bdc1) into master (4e7f2d4) will decrease coverage by 23.8%.
The diff coverage is 40.0%.

@@            Coverage Diff            @@
##           master    #602      +/-   ##
=========================================
- Coverage    57.8%   34.0%   -23.7%     
=========================================
  Files          39      39              
  Lines        2497    2477      -20     
  Branches       73      71       -2     
=========================================
- Hits         1441     840     -601     
- Misses       1041    1566     +525     
- Partials       15      71      +56     
Flag Coverage Δ
mypy 34.0% <40.0%> (+0.1%) ⬆️
tests ?

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

Impacted Files Coverage Δ
django_redis/client/default.py 39.0% <33.4%> (-51.4%) ⬇️
tests/test_backend.py 15.2% <44.5%> (+0.5%) ⬆️
django_redis/client/sharded.py 5.9% <0.0%> (-80.1%) ⬇️
django_redis/pool.py 11.0% <0.0%> (-66.0%) ⬇️
django_redis/cache.py 34.2% <0.0%> (-62.8%) ⬇️
django_redis/serializers/msgpack.py 37.5% <0.0%> (-62.5%) ⬇️
django_redis/client/sentinel.py 22.8% <0.0%> (-54.5%) ⬇️
django_redis/serializers/base.py 50.0% <0.0%> (-50.0%) ⬇️
django_redis/serializers/pickle.py 54.6% <0.0%> (-45.4%) ⬇️
... and 9 more

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 4e7f2d4...7c3bdc1. Read the comment docs.

@syphar
Copy link

syphar commented Apr 18, 2022

This is more a design question, so not to me to decide, but:

In my mind this is not the solution but only a workaround. Users of the library have to know that they need to use enforce_encoding when they want to cache specific data-types.

Since we're already special-casing bool, because isinstance(bool, int) is True,
why not just adding a special case enum.Enum too?

@syphar for some strange reason I can't add you in the reviewers and it did not even suggest you when I was tagging you

Only users part of the repo can be added as reviewers, and are in the suggestion.

@WisdomPill
Copy link
Member Author

I like your idea, I would happily change it. I want to point out what I think is the reason why there are this exceptions. I think it is because of space, numbers and booleans use much more space than pickle serialized objects.

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.

reading cache breaks with int-enums with redis-py >= 4.0
2 participants