-
Notifications
You must be signed in to change notification settings - Fork 70
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
Add support for conversion to pyo3::PyErr #47
Conversation
It looks like my autoformatter made a few changes to the Cargo.toml - do you want me to revert those? |
No need, I'd be happy to have the formatting there made more consistent. The PR looks good to me though I have one concern still I want to dig into before we go ahead and merge this. Namely, will people want to be able to round-trip |
A review from the maintainer of pyo3 would be the best way forward if he is available, I think. |
As written, this seems reasonable enough.
That's a great question. So the most obvious way would be to make an exception I think we will need to at the very least implement PyO3/pyo3#295 to get a nice round-tripping API. |
I think since round tripping seems like it would add some additional burden on the user, it should be fine to just proceed with stringification for now. If users really care about round tripping, they can choose to manually convert to PyErr themselves instead of using the provided implementation. |
👍 especially once the above linked pyo3 issue is implemented, it should be easy for users to shove |
sounds good, I went ahead and fixed some CI issues because |
Lgtm. Thanks for the effort on this! |
@TheButlah |
closes #45