Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32.4k
bpo-31116: Add Z85 variant to base64#30598
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
This PR is stale because it has been open for 30 days with no activity. |
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.
This needs some documentation a single short NEWS entry I don't think is enough. A short description plus maybe a link to reference.
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 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 updateversionadded 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 |
Lib/test/test_base64.py Outdated
| 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') |
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.
The first four should be prefixes ofb'%nSc0' which is a Z85 encodedb'\xff\xff\xff\xff'. The last one should beb'%nSc1', it represents 0x100000000 which cannot be packed in 4 bytes.
Perhaps it needs a comment.
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.
Fixed, Thanks!
Lib/test/test_base64.py Outdated
| 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: |
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.
| # 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: |
Uh oh!
There was an error while loading.Please reload this page.
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.
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.
Uh oh!
There was an error while loading.Please reload this page.
Thank you for your contribution@matan1008. |
Thank you for accepting it! |
Uh oh!
There was an error while loading.Please reload this page.
https://bugs.python.org/issue31116