bpo-31116: Add Z85 variant to base64#30598
Conversation
|
This PR is stale because it has been open for 30 days with no activity. |
MaxwellDupre
left a comment
There was a problem hiding this comment.
This needs some documentation a single short NEWS entry I don't think is enough. A short description plus maybe a link to reference.
serhiy-storchaka
left a comment
There was a problem hiding this comment.
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 |
b7ae824 to
a14b45f
Compare
|
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.
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.
a14b45f to
4165ba3
Compare
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.
| # 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: |
4165ba3 to
7636618
Compare
serhiy-storchaka
left a comment
There was a problem hiding this comment.
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 your contribution @matan1008. |
|
Thank you for accepting it! |
https://bugs.python.org/issue31116