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

Change pickle default version to Python default instead of highest version #555

Merged
merged 1 commit into from
Oct 20, 2021

Conversation

rootart
Copy link
Contributor

@rootart rootart commented Oct 14, 2021

Fixes: #547 regarding using pickle.DEFAULT_PROTOCOL as the default pickle version.

@codecov
Copy link

codecov bot commented Oct 14, 2021

Codecov Report

Merging #555 (8da2f64) into master (f2fd5f0) will increase coverage by 0.3%.
The diff coverage is 100.0%.

Impacted file tree graph

@@           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             
Flag Coverage Δ
mypy 33.7% <100.0%> (+0.3%) ⬆️
tests 83.1% <100.0%> (+0.5%) ⬆️

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

Impacted Files Coverage Δ
django_redis/serializers/pickle.py 100.0% <100.0%> (+10.6%) ⬆️

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 f2fd5f0...8da2f64. Read the comment docs.

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.

Small fix is needed for future versions of pickle

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})
Copy link
Member

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

Suggested change
serializer_p5 = PickleSerializer({"PICKLE_VERSION": pickle.HIGHEST_PROTOCOL})
serializer_p5 = PickleSerializer({"PICKLE_VERSION": 5})

Copy link
Contributor

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.

Copy link
Member

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

Copy link
Contributor

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.

@WisdomPill
Copy link
Member

By the way, thank you very much for your contribution @rootart!!

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})
Copy link
Contributor

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.

Comment on lines 11 to 14
@pytest.mark.skipif(
sys.version_info < (3, 8),
reason="Protocol 5 was added as highest from python version 3.8",
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
@pytest.mark.skipif(
sys.version_info < (3, 8),
reason="Protocol 5 was added as highest from python version 3.8",
)

@rootart
Copy link
Contributor Author

rootart commented Oct 20, 2021

By the way, thank you very much for your contribution @rootart!!
@WisdomPill it is my pleasure :) thank you for the reviews!

@WisdomPill WisdomPill merged commit 52ba679 into jazzband:master Oct 20, 2021
millerdev added a commit to dimagi/commcare-hq that referenced this pull request Jul 18, 2023
Django-redis has been upgraded to a version that uses `pickle.DEFAULT_PROTOCOL` instead of `pickle.HIGHEST_PROTOCOL`. See jazzband/django-redis#555
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.

Change pickle default version to Python default
4 participants