bpo-31116: Add Z85 variant to base64 by matan1008 · Pull Request #30598 · python/cpython
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!
