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

Unwrap dynamic error types when inner is simple PyErr #3004

Merged
merged 1 commit into from
May 9, 2023

Conversation

BlueGlassBlock
Copy link
Contributor

This is the first part of suggested improvements in #2998.

This change will make bubbled PyErr wrapped in eyre::Report / anyhow::Error bubble up unchanged, instead of being wrapped in a PyRuntimeError.

Copy link
Member

@adamreichold adamreichold left a comment

Choose a reason for hiding this comment

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

Thanks! I think this is a nice improvement with minimal effort.

src/conversions/eyre.rs Outdated Show resolved Hide resolved
@adamreichold
Copy link
Member

@mejrs Will wait for your feedback before merging as you sounded unsure whether this is a sensible thing to do in the issue.

Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

While I think this change is strictly an improvement, I'd like to quickly take a moment to think about how we document this and / or provide backwards compatibility.

Changing the error type observed here may catch some users out. Probably module docs need updating. Should this go in the migration guide?

src/conversions/anyhow.rs Show resolved Hide resolved
src/conversions/anyhow.rs Outdated Show resolved Hide resolved
@DataTriny
Copy link
Contributor

DataTriny commented Apr 16, 2023

First time contributor has agreed to the new licensing scheme.

@mejrs
Copy link
Member

mejrs commented Apr 20, 2023

@mejrs Will wait for your feedback before merging as you sounded unsure whether this is a sensible thing to do in the issue.

I have nothing, feel free to merge as you wish :)

@adamreichold
Copy link
Member

@BlueGlassBlock You would be up to writing an entry for the migration guide showing the difference when forwarding errors using anyhow or eyre? The module docs definitely must be changed to match the proposed implementation.

@BlueGlassBlock
Copy link
Contributor Author

@BlueGlassBlock You would be up to writing an entry for the migration guide showing the difference when forwarding errors using anyhow or eyre? The module docs definitely must be changed to match the proposed implementation.

Certainly!

guide/src/migration.md Outdated Show resolved Hide resolved
guide/src/migration.md Outdated Show resolved Hide resolved
@adamreichold
Copy link
Member

Thanks for drafting the migration guide entry. Please remember to adjust the existing module-level docs in anyhow.rs and eyre.rs which currently still state that a RuntimeError is produced unconditionally.

guide/src/migration.md Outdated Show resolved Hide resolved
@adamreichold
Copy link
Member

Changing the error type observed here may catch some users out. Probably module docs need updating. Should this go in the migration guide?

@davidhewitt I think this better positioned now w.r.t. the above concerns and therefore ready to be merged. Do you agree?

Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

Yep this looks great to me now, thanks! I'll squash and merge this now.

@davidhewitt
Copy link
Member

bors r=davidhewitt,adamreichold,mejrs

@bors
Copy link
Contributor

bors bot commented May 9, 2023

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

  • conclusion

@bors bors bot merged commit 6281b47 into PyO3:main May 9, 2023
@BlueGlassBlock BlueGlassBlock deleted the dyn_err branch May 9, 2023 09:28
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.

None yet

5 participants