GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — bpo-31116: Add Z85 variant to base64 by matan1008 · Pull Request #30598 · python/cpython

Conversation

matan1008 commented Jan 14, 2022 •

edited by serhiy-storchaka

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

https://bugs.python.org/issue31116

base85 z85 variant encoding #75299

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          bpo-31116: Add Z85 variant to base64

bb03146

the-knights-who-say-ni added the CLA signed label

Jan 14, 2022

bedevere-bot added the awaiting review label

Jan 14, 2022


          📜🤖 Added by blurb_it.

d0f2d7c

github-actions Bot commented Feb 14, 2022

Copy link

Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

github-actions Bot added the stale Stale PR or inactive for long period of time. label

Feb 14, 2022

MaxwellDupre suggested changes

Feb 25, 2022

View reviewed changes

MaxwellDupre left a comment

Copy link

Copy Markdown

Contributor

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

matan1008 reacted with thumbs up emoji

All reactions

👍 1 reaction

bedevere-bot added awaiting core review and removed awaiting review labels

Feb 25, 2022

matan1008 added 2 commits

March 5, 2022 20:51


          bpo-31116: Add documentation

d65a8a1


          bpo-31116: Fix newline at end of NEWS entry

ca6999a

ezio-melotti removed the CLA signed label

Jul 13, 2022


          Merge branch 'main' into add-z85-variant

e1d969c

serhiy-storchaka requested changes

Feb 24, 2024

View reviewed changes

serhiy-storchaka left a comment

Copy link

Copy Markdown

Member

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 update versionadded directives.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

matan1008 reacted with thumbs up emoji

All reactions

👍 1 reaction

bedevere-app Bot added awaiting changes and removed awaiting core review labels

Feb 24, 2024

bedevere-app Bot commented Feb 24, 2024

Copy link

Copy Markdown

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

matan1008 force-pushed the add-z85-variant branch from b7ae824 to a14b45f Compare

February 25, 2024 07:45

matan1008 commented Feb 25, 2024

Copy link

Copy Markdown

Contributor Author

I have made the requested changes; please review again

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

serhiy-storchaka reviewed

Feb 25, 2024

View reviewed changes

Comment thread

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')

serhiy-storchaka Feb 25, 2024

Copy link

Copy Markdown

Member

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 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

matan1008 Feb 25, 2024

Copy link

Copy Markdown

Contributor Author

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!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

serhiy-storchaka removed the stale Stale PR or inactive for long period of time. label

Feb 25, 2024

matan1008 force-pushed the add-z85-variant branch from a14b45f to 4165ba3 Compare

February 25, 2024 11:39

serhiy-storchaka reviewed

Feb 25, 2024

View reviewed changes

Comment thread

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:

serhiy-storchaka Feb 25, 2024

Copy link

Copy Markdown

Member

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.

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:

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

matan1008 reacted with thumbs up emoji

All reactions

👍 1 reaction

matan1008 added 3 commits

February 25, 2024 14:11


          bpo-31116: Handle z85 errors

c87c77e


          bpo-31116: Update versionadded directives

4c634f3


          bpo-31116: Add z85 feature to whatsnew

matan1008 force-pushed the add-z85-variant branch from 4165ba3 to 7636618 Compare

February 25, 2024 12:12

serhiy-storchaka reviewed

Feb 25, 2024

View reviewed changes

Comment thread

Doc/whatsnew/3.13.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.


          Update Doc/whatsnew/3.13.rst

503b2b3

serhiy-storchaka approved these changes

Feb 25, 2024

View reviewed changes

serhiy-storchaka left a comment

Copy link

Copy Markdown

Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

matan1008 reacted with thumbs up emoji

All reactions

👍 1 reaction

bedevere-app Bot added awaiting merge and removed awaiting changes labels

Feb 25, 2024

serhiy-storchaka reviewed

Feb 25, 2024

View reviewed changes

Comment thread

Doc/whatsnew/3.13.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

serhiy-storchaka added 2 commits

February 25, 2024 14:43


          Update Doc/whatsnew/3.13.rst

a3d375f


          Merge branch 'main' into add-z85-variant

5b95714

serhiy-storchaka merged commit c40b5b9 into python:main

Feb 25, 2024

bedevere-app Bot removed the awaiting merge label

Feb 25, 2024

underrun mannequin mentioned this pull request

Apr 10, 2022

base85 z85 variant encoding #75299

Closed

serhiy-storchaka commented Feb 25, 2024

Copy link

Copy Markdown

Member

Thank you for your contribution @matan1008.

matan1008 reacted with hooray emoji

All reactions

🎉 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

matan1008 commented Feb 25, 2024

Copy link

Copy Markdown

Contributor Author

Thank you for accepting it!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

woodruffw pushed a commit to woodruffw-forks/cpython that referenced this pull request

Mar 4, 2024


          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


          bpo-31116: Add Z85 variant to base64 (pythonGH-30598)

Z85  specification: https://rfc.zeromq.org/spec/32/

LukasWoodtli pushed a commit to LukasWoodtli/cpython that referenced this pull request

Jan 22, 2025


          bpo-31116: Add Z85 variant to base64 (pythonGH-30598)

5aeeb1e

Z85  specification: https://rfc.zeromq.org/spec/32/

Reviewers

serhiy-storchaka

serhiy-storchaka approved these changes

+1 more reviewer

MaxwellDupre

MaxwellDupre requested changes

Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

Conversation

matan1008 commented Jan 14, 2022 • edited by serhiy-storchaka Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Feb 14, 2022

Uh oh!

MaxwellDupre left a comment

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

bedevere-app Bot commented Feb 24, 2024

Uh oh!

matan1008 commented Feb 25, 2024

Uh oh!

serhiy-storchaka Feb 25, 2024

Choose a reason for hiding this comment

Uh oh!

matan1008 Feb 25, 2024

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka Feb 25, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

serhiy-storchaka commented Feb 25, 2024

Uh oh!

matan1008 commented Feb 25, 2024

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

matan1008 commented Jan 14, 2022 •

edited by serhiy-storchaka

Loading

Uh oh!

There was an error while loading. Please reload this page.