-
-
Notifications
You must be signed in to change notification settings - Fork 429
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
Change pickle default version to Python default instead of highest version #555
Conversation
684883d
to
a894611
Compare
Codecov Report
@@ Coverage Diff @@
## master #555 +/- ##
========================================
+ Coverage 83.6% 83.8% +0.3%
========================================
Files 21 21
Lines 1171 1175 +4
Branches 60 60
========================================
+ Hits 978 984 +6
+ Misses 187 185 -2
Partials 6 6
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small fix is needed for future versions of pickle
tests/test_serializers.py
Outdated
reason="Protocol 5 was added as highest from python version 3.8", | ||
) | ||
def test_using_negative_protocol_value(self): | ||
serializer_p5 = PickleSerializer({"PICKLE_VERSION": pickle.HIGHEST_PROTOCOL}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
better to specify version 5 so that it will keep passing even when there will an eventual version 6 in the future
serializer_p5 = PickleSerializer({"PICKLE_VERSION": pickle.HIGHEST_PROTOCOL}) | |
serializer_p5 = PickleSerializer({"PICKLE_VERSION": 5}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-1 is always the highest protocol, but I'm a little confused why there's a skip for < Python 3.8. This should be valid for all Python versions and it's fine to just run the test everywhere instead of special casing this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I missed it, you are right, but then the assertion should be on the pickle_version used or the test does not make sense
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I agree. I'm fine removing the test since it's actually just tesitng that -1 is interpreted as HIGEST_VERSION
by pickle
and that's outside the domain of this package.
By the way, thank you very much for your contribution @rootart!! |
tests/test_serializers.py
Outdated
reason="Protocol 5 was added as highest from python version 3.8", | ||
) | ||
def test_using_negative_protocol_value(self): | ||
serializer_p5 = PickleSerializer({"PICKLE_VERSION": pickle.HIGHEST_PROTOCOL}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-1 is always the highest protocol, but I'm a little confused why there's a skip for < Python 3.8. This should be valid for all Python versions and it's fine to just run the test everywhere instead of special casing this.
tests/test_serializers.py
Outdated
@pytest.mark.skipif( | ||
sys.version_info < (3, 8), | ||
reason="Protocol 5 was added as highest from python version 3.8", | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pytest.mark.skipif( | |
sys.version_info < (3, 8), | |
reason="Protocol 5 was added as highest from python version 3.8", | |
) |
a894611
to
8da2f64
Compare
|
Django-redis has been upgraded to a version that uses `pickle.DEFAULT_PROTOCOL` instead of `pickle.HIGHEST_PROTOCOL`. See jazzband/django-redis#555
Fixes: #547 regarding using
pickle.DEFAULT_PROTOCOL
as the default pickle version.