-
-
Notifications
You must be signed in to change notification settings - Fork 34k
gh-144001: Support ignorechars in binascii.a2b_base64() and base64.b64decode() #144024
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
gh-144001: Support ignorechars in binascii.a2b_base64() and base64.b64decode() #144024
Conversation
|
This is a draft because I am going to apply the error handling changes in a separate issue first. We can also apply some optimizations -- cache the ignored chars and returning on fast path after ignored characters. |
1b0bb87 to
192d535
Compare
|
Well, the issue with error handling was just that the error message could be more specific in some cases. It is not a bug. It can be included in this PR. |
192d535 to
4f3847e
Compare
|
No worries, that was the point I was just free on Sunday, so I push out a PR in case you haven't started on it. BTW your PR looks good, I am no maintainer but you just have to run |
vstinner
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.
I'm fine with changing validate default to True if ignorechars is specified. The change produces more precise error messages. It also adds a lot of tests which is always a good thing :-)
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
|
Taking a quick look: The docs in this PR changed the default for validate and strict_mode from False to True. But versionchanged does not mention that change and there was no deprecation period. Did this behavior change? The docs need fixing at a minimum but if the behavior changed we also need a deprecation cycle. What I understand @vstinner above to be saying is to change the default only in the case of the new parameter being supplied? |
|
that may just be a simple doc fix. the declaration lines need to not state a value for the parameters who's default behavior varies based on context: The text below describes a dynamic behavior on both, but people will absolutely misread the function signature and not read the text if we do not indicate it in the signature. |
|
Sorry, I do not understand. How is it different from what is in the documentation now? |
|
Deprecation period is needed if we want to change the existing behavior into error. But |
The default remains |
I like that. It's quick to read. |
📚 Documentation preview 📚: https://cpython-previews--144024.org.readthedocs.build/