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

Add support for conversion to pyo3::PyErr #47

Merged
merged 4 commits into from
Jan 5, 2021
Merged

Add support for conversion to pyo3::PyErr #47

merged 4 commits into from
Jan 5, 2021

Conversation

TheButlah
Copy link
Contributor

closes #45

@TheButlah
Copy link
Contributor Author

It looks like my autoformatter made a few changes to the Cargo.toml - do you want me to revert those?

@yaahc
Copy link
Collaborator

yaahc commented Jan 4, 2021

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 eyre::Reports through python and if so, is this something we can reasonably support? I don't love the idea of just stringifying an eyre::Report when it goes to python land because we lose so much information that way, but looking at pyo3 this already seems to be pretty standard so maybe its fine? We should probably also get a quick review from @davidhewitt as well, since they're much more familiar with the pyo3 side of things.

@TheButlah
Copy link
Contributor Author

A review from the maintainer of pyo3 would be the best way forward if he is available, I think.

@davidhewitt
Copy link

As written, this seems reasonable enough. impl From<eyre::Report> for PyErr will allow users to write #[pymethods] and #[pyfunction] implementations which return eyre::Result, and under the hood PyO3 will cast the errors if needed.

Namely, will people want to be able to round-trip eyre::Reports through python and if so, is this something we can reasonably support?

That's a great question. So the most obvious way would be to make an exception #[pyclass] which contains an eyre::Report - and you'd have to create a PyErr from that excepction. It's a good data point to consider. Something that'll be a little weird about it is that I think each pyo3 extension using eyre would probably have to export their own copy of this class (or eyre would need to provide its own python cdylib which could be linked against with the shared class definition).

I think we will need to at the very least implement PyO3/pyo3#295 to get a nice round-tripping API.

@TheButlah
Copy link
Contributor Author

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.

@davidhewitt
Copy link

👍 especially once the above linked pyo3 issue is implemented, it should be easy for users to shove eyre::Report in their own #[pyexception] types.

@yaahc
Copy link
Collaborator

yaahc commented Jan 4, 2021

sounds good, I went ahead and fixed some CI issues because eyre support's rust back to 1.39 but pyo3 doesn't compile on that version (which is fine). I also changed the organization a little bit so the pyo3 code is in its own module. should be good to merge and cut a new release, @TheButlah lmk if you see any issues with the changes I pushed, if it looks good to you I'll go ahead and merge + release.

@TheButlah
Copy link
Contributor Author

TheButlah commented Jan 5, 2021

Lgtm. Thanks for the effort on this!

@yaahc yaahc merged commit 1ea7c20 into eyre-rs:master Jan 5, 2021
@yaahc
Copy link
Collaborator

yaahc commented Jan 5, 2021

@TheButlah 0.6.5 should be available now with your changes

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.

[Feature Request] Support conversion to pyo3::PyErr
3 participants