-
-
Notifications
You must be signed in to change notification settings - Fork 31.4k
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
bpo-31271: fix an assertion failure in io.TextIOWrapper.write #3201
bpo-31271: fix an assertion failure in io.TextIOWrapper.write #3201
Conversation
Lib/test/test_io.py
Outdated
return BadEncoder() | ||
quopri = codecs.lookup("quopri") | ||
with support.swap_attr(quopri, '_is_text_encoding', True), \ | ||
support.swap_attr(quopri, 'incrementalencoder', _get_bad_encoder): |
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.
Is monkey-patching incrementalencoder needed?
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 not sure I understood your question.
- if we remove it, then the assertion won't fail (even without the issue's patch).
- if we just overwrite quopri.incrementalencoder, without restoring it, the assertion would fail (without the issue's patch), but I am not sure about the effects of not restoring it (on Python with the issue's patch), when Python keeps running.
- can we reproduce the assertion failure without changing quopri.incrementalencoder? I don't know. I haven't looked for any other way, but I can do that, if you think it's worth it.
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.
What if use an existing encoding that produces non-bytes? E.g. "rot13"?
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.
indeed. I would update the patch.
is a NEWS.d entry necessary? |
This would not hurt. |
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.
Fix looks good to me as well, but the automated check is correct that this does need a NEWS entry.
…ilar error when the decoder doesn't return a str.
…ythonGH-3201) (cherry picked from commit a5b4ea1)
…ythonGH-3201) (cherry picked from commit a5b4ea1)
GH-3209 is a backport of this pull request to the 3.6 branch. |
Thanks @orenmn for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 2.7. |
…ythonGH-3201) (cherry picked from commit a5b4ea1)
GH-3548 is a backport of this pull request to the 2.7 branch. |
Thanks @orenmn for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 2.7. |
…ythonGH-3201) (cherry picked from commit a5b4ea1)
GH-3951 is a backport of this pull request to the 2.7 branch. |
https://bugs.python.org/issue31271