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.

Sign up for GitHub

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

Jump to bottom

bpo-31116: Add Z85 variant to base64 #30598

Merged
merged 11 commits into from
Feb 25, 2024
Merged

bpo-31116: Add Z85 variant to base64 #30598

merged 11 commits into from
Feb 25, 2024

Conversation

matan1008
Copy link
Contributor

@matan1008 matan1008 commented Jan 14, 2022 •
edited by serhiy-storchaka
Loading

@github-actions
Copy link

This PR is stale because it has been open for 30 days with no activity.

@github-actions github-actions bot added the stale Stale PR or inactive for long period of time. label Feb 14, 2022
MaxwellDupre
MaxwellDupre suggested changes Feb 25, 2022
View reviewed changes
Copy link
Contributor

@MaxwellDupre MaxwellDupre left a 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.

serhiy-storchaka
serhiy-storchaka requested changes Feb 24, 2024
View reviewed changes
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a 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 update versionadded directives.

@bedevere-app
Copy link

bedevere-app bot commented Feb 24, 2024

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.

@matan1008
Copy link
Contributor Author

I have made the requested changes; please review again

serhiy-storchaka
serhiy-storchaka reviewed Feb 25, 2024
View reviewed changes
Lib/test/test_base64.py Outdated
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')
Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed, Thanks!

@serhiy-storchaka serhiy-storchaka removed the stale Stale PR or inactive for long period of time. label Feb 25, 2024
serhiy-storchaka
serhiy-storchaka reviewed Feb 25, 2024
View reviewed changes
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:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# 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:

serhiy-storchaka
serhiy-storchaka approved these changes Feb 25, 2024
View reviewed changes
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a 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.

@serhiy-storchaka serhiy-storchaka merged commit c40b5b9 into python:main Feb 25, 2024
31 checks passed
@underrun underrun mannequin mentioned this pull request Apr 10, 2022
Closed
@serhiy-storchaka
Copy link
Member

Thank you for your contribution @matan1008.

@matan1008
Copy link
Contributor Author

Thank you for accepting it!

woodruffw pushed a commit to woodruffw-forks/cpython that referenced this pull request Mar 4, 2024
@matan1008 @woodruffw
bpo-31116: Add Z85 variant to base64 (pythonGH-30598)
94b7226 
Z85  specification: https://rfc.zeromq.org/spec/32/
diegorusso pushed a commit to diegorusso/cpython that referenced this pull request Apr 17, 2024
@matan1008 @diegorusso
bpo-31116: Add Z85 variant to base64 (pythonGH-30598)
8849687 
Z85  specification: https://rfc.zeromq.org/spec/32/
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes

@MaxwellDupre MaxwellDupre MaxwellDupre requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@matan1008 @serhiy-storchaka @MaxwellDupre @the-knights-who-say-ni @ezio-melotti @bedevere-bot