Repository navigation
gh-143103: Added pad parameter to base64.z85encode - #143106
serhiy-storchaka merged 1 commit into
Conversation
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
picnixz
left a comment
There was a problem hiding this comment.
I am reviewing on my phone so check my suggestions before committing them (like typos or things like that).
Sorry, something went wrong.
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.
|
@picnixz Thanks, I applied the changes and force-pushed the squashed result. |
Sorry, something went wrong.
|
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) |
Sorry, something went wrong.
|
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 |
Sorry, something went wrong.
serhiy-storchaka
left a comment
There was a problem hiding this comment.
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.
Sorry, something went wrong.
|
AFAIK, not all minor changes deserve an entry in What's New. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
Yeah but I think it is nice to have features there. Otherwise they get burried in the bugfixes as well |
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
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. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
…143106) 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.
This makes z85encode 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. 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/