Skip to content

Add check if str(handle) correctly converted the object, and throw py::error_already_set if not (bis) #2477

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

Merged

Conversation

YannickJadoul
Copy link
Collaborator

Same as #2392, already reviewed and merged into str-bytes-cleanup.
This are just the original commits cherry-picked, plus one additional fix to the tests (since #2380 is on str-bytes-cleanup but not on master yet).

@YannickJadoul
Copy link
Collaborator Author

@rwgk, yes, you were right in #2473: it is messy to get the old commits, since they are more intertwined with the other fixes. Apologies, #2473 should not per se have been closed, because I'm now ending up with basically the same PR.

But here we are, anyway. Since this already passed review, go ahead and merge this (I think it's the same as you had, except that I referred to the chimera-PR and left the intended test in place).

@YannickJadoul YannickJadoul changed the title Fix python2 str malformed utf8 Add check if str(handle) correctly converted the object, and throw py::error_already_set if not (bis) Sep 10, 2020
Copy link
Collaborator

@rwgk rwgk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one minor nit. Please go ahead merging this PR.

@YannickJadoul YannickJadoul force-pushed the fix-python2-str-malformed_utf8 branch from 76f3ebe to 055f86f Compare September 11, 2020 16:23
@YannickJadoul YannickJadoul merged commit fe9ee86 into pybind:master Sep 11, 2020
@rwgk
Copy link
Collaborator

rwgk commented Sep 11, 2020

Awesome @YannickJadoul, thanks for merging!

@YannickJadoul
Copy link
Collaborator Author

Thanks for the reminder!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants