Skip to content

Conversation

@orenmn
Copy link
Contributor

@orenmn orenmn commented Aug 24, 2017

  • in textio.c: add a check whether encoder's encode() returned a bytes object.
  • in test_io.py: add a test to verify the assertion doesn't fail anymore.

https://bugs.python.org/issue31271

return BadEncoder()
quopri = codecs.lookup("quopri")
with support.swap_attr(quopri, '_is_text_encoding', True), \
support.swap_attr(quopri, 'incrementalencoder', _get_bad_encoder):
Copy link
Member

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?

Copy link
Contributor Author

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.
Copy link
Member

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"?

Copy link
Contributor Author

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.

@orenmn
Copy link
Contributor Author

orenmn commented Aug 25, 2017

is a NEWS.d entry necessary?

@serhiy-storchaka
Copy link
Member

This would not hurt.

Copy link
Contributor

@ncoghlan ncoghlan left a 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.
@serhiy-storchaka serhiy-storchaka merged commit a5b4ea1 into python:master Aug 25, 2017
orenmn added a commit to orenmn/cpython that referenced this pull request Aug 26, 2017
orenmn added a commit to orenmn/cpython that referenced this pull request Aug 26, 2017
@bedevere-bot
Copy link

GH-3209 is a backport of this pull request to the 3.6 branch.

@miss-islington
Copy link
Contributor

Thanks @orenmn for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 2.7.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Sep 13, 2017
@bedevere-bot
Copy link

GH-3548 is a backport of this pull request to the 2.7 branch.

@miss-islington
Copy link
Contributor

Thanks @orenmn for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 2.7.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Sep 16, 2017
@bedevere-bot
Copy link

bedevere-bot commented Sep 16, 2017

GH-3951 is a backport of this pull request to the 2.7 branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

7 participants