bpo-31116: Add Z85 variant to base64 by matan1008 · Pull Request #30598 · python/cpython (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation13 Commits11 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
Contributor
•
edited by serhiy-storchaka
Loading
This PR is stale because it has been open for 30 days with no activity.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs some documentation a single short NEWS entry I don't think is enough. A short description plus maybe a link to reference.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I apologize for the fact that this PR was not reviewed earlier. The idea itself looks reasonable to me, and the implementation looks clever, so I am going to approve this change.
Please add a variant of test_b85decode_errors for Z85. I suspect that there may be some issues here. Also please add a What's New entry and update versionadded directives.
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.
Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.
I have made the requested changes; please review again
Comment on lines 787 to 791
| self.assertRaises(ValueError, base64.z85decode, b'%') |
|---|
| self.assertRaises(ValueError, base64.z85decode, b'%N') |
| self.assertRaises(ValueError, base64.z85decode, b'%Ns') |
| self.assertRaises(ValueError, base64.z85decode, b'%NsC') |
| self.assertRaises(ValueError, base64.z85decode, b'%NsC1') |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The first four should be prefixes of b'%nSc0' which is a Z85 encoded b'\xff\xff\xff\xff'. The last one should be b'%nSc1', it represents 0x100000000 which cannot be packed in 4 bytes.
Perhaps it needs a comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed, Thanks!
| with self.assertRaises(ValueError, msg=bytes([c])): |
|---|
| base64.z85decode(b'0000' + bytes([c])) |
| # b'\xff\xff\xff\xff' encodes to b'%nSc1', the following will overflow: |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| # b'\xff\xff\xff\xff' encodes to b'%nSc1', the following will overflow: |
|---|
| # b'\xff\xff\xff\xff' encodes to b'%nSc0', the following will overflow: |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
For future, please do not use force push during review. It makes more difficult to see the incremental changes and requires to start reviewing from the start after every change.
Thank you for accepting it!
woodruffw pushed a commit to woodruffw-forks/cpython that referenced this pull request
diegorusso pushed a commit to diegorusso/cpython that referenced this pull request
LukasWoodtli pushed a commit to LukasWoodtli/cpython that referenced this pull request
