Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 33.9k
gh-143103: Added pad parameter to base64.z85encode#143106
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 our terms of service and privacy 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
haukex commented Dec 23, 2025 • edited by github-actions bot
Loading Uh oh!
There was an error while loading. Please reload this page.
edited by github-actions bot
Uh oh!
There was an error while loading. Please reload this page.
Misc/NEWS.d/next/Library/2025-12-23-17-07-22.gh-issue-143103.LRjXEW.rst Outdated Show resolvedHide resolved
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
pad argument to base64.z85encodepad parameter to base64.z85encodehaukex commented Dec 23, 2025
All suggested changes applied including correcting "argument" to "parameter" in docs and commit message as well; the force push was just a squash of those changes. |
picnixz left 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.
I am reviewing on my phone so check my suggestions before committing them (like typos or things like that).
Uh oh!
There was an error while loading. Please reload this page.
Misc/NEWS.d/next/Library/2025-12-23-17-07-22.gh-issue-143103.LRjXEW.rst Outdated Show resolvedHide resolved
Uh oh!
There was an error while loading. Please reload this page.
This makes it analogous to `a85encode` and `b85encode` and allows the user to more easily meet the Z85 specification, which requires input lengths to be a multiple of 4.
haukex commented Dec 24, 2025
@picnixz Thanks, I applied the changes and force-pushed the squashed result. |
picnixz commented Dec 24, 2025
Thanks. In the future do not force push. We squash everything at the end so it doesn't help (it however gets harder for incremental reviews). Force-pushing is reserved when the history got messed up (e.g. bad merge) |
picnixz commented Dec 24, 2025
Looks good but can you add a What's New entry as well for this change? just restate what you did in the NEWS entry (look at Doc/whatsnew/3.15.rst to see what to do). TiA |
serhiy-storchaka left 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.
LGTM. 👍
Side note. I thought about proposing to make pad a keyword-only parameter. Boolean arguments almost always should be passed by keyword, otherwise it is errorprone if you pass many arguments. This is not applied when the number of parameters is small (like in str.splitlines()), and this may be one of such cases. So I am not sure.
serhiy-storchaka commented Dec 24, 2025
AFAIK, not all minor changes deserve an entry in What's New. |
picnixz commented Dec 24, 2025
I also wanted to suggest a kw-only but b85* functions do not have a kw-only and I think it is better to keep similar signatures for the encoding functions. |
picnixz commented Dec 24, 2025
Yeah but I think it is nice to have features there. Otherwise they get burried in the bugfixes as well |
picnixz left a comment • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
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.
I also approve this. If you do not want to make the What's New entry it is fine. We can always do it as part of our copy-edit issue. It will also be easier so that you do not need to be careful about where to put the entry and how to format it.
haukex commented Dec 25, 2025
Ok, understood!
The change is pretty small, so I think I'd leave the decision as to whether it's worth adding to the list or not up to you if that's ok. Thanks very much @serhiy-storchaka and @picnixz! Then the PR is good to go from my side as well. |
8d46f96 into python:mainUh oh!
There was an error while loading. Please reload this page.
serhiy-storchaka commented Dec 27, 2025
I will add a What's New entry in #143216. I am planning to add more parameters, so in sum they will be not so small change. |
This makes
z85encodeanalogous toa85encodeandb85encodeand allows the user to more easily meet the Z85 specification, which requires input lengths to be a multiple of 4. For details see #143103.Documentation and tests included. Tests were verified against the Z85 reference implementation (which I wrapped into a Python module at haukex/py-z85).
📚 Documentation preview 📚: https://cpython-previews--143106.org.readthedocs.build/