Skip to content
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

Raise codec errors when casting to std::string #2903

Merged
merged 5 commits into from
Jul 14, 2021

Conversation

sloretz
Copy link
Contributor

@sloretz sloretz commented Mar 15, 2021

Description

This allows the codec's exception to be raised instead of RuntimeError when casting from py::str to std::string. I ran into this when porting a CPython extention to pybind11 with an existing test that expected a UnicodeEncodeError when given a string with UCS surrogate characters.

Suggested changelog entry:

Allow the codec's exception to be raised instead of :code:`RuntimeError` when casting from :code:`py::str` to :code:`std::string`.

@sloretz sloretz force-pushed the string_cast_unicode_error branch from 1b02928 to e1abcb6 Compare March 15, 2021 18:35
Allow the codec's exception to be raised instead of RuntimeError when
casting from py::str to std::string.

PY2 allows ucs surrogates in UTF-8 conversion

Signed-off-by: Shane Loretz <[email protected]>
Signed-off-by: Shane Loretz <[email protected]>
@sloretz sloretz force-pushed the string_cast_unicode_error branch from e1abcb6 to 8dd8461 Compare March 15, 2021 19:05
@Skylion007 Skylion007 requested a review from rwgk July 13, 2021 16:58
@rwgk
Copy link
Collaborator

rwgk commented Jul 13, 2021

Hi @Skylion007, consider this approved also by me, but ideally I'd run this through the Google-internal global testing (which includes testing a large number of open source packages) before merging. Is it OK with you to wait a few hours?

In the past we had a couple cases with a surprising number of hiccups caused by similar changes. I feel this one will be fine, but good to be sure by testing first.

@Skylion007
Copy link
Collaborator

@rwgk Sure, go ahead.

@rwgk
Copy link
Collaborator

rwgk commented Jul 14, 2021

@rwgk Sure, go ahead.

The testing just finished: no issues!

I'll go ahead with merging this PR. Thanks everybody! And sorry @sloretz this took us so long.

@rwgk rwgk merged commit 7331d38 into pybind:master Jul 14, 2021
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label Jul 14, 2021
@henryiii henryiii removed the needs changelog Possibly needs a changelog entry label Jul 16, 2021
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.

5 participants