Skip to content

Conversation

@Jazzinghen
Copy link
Contributor

Updated Context.close and Context.__exit__ to allow forwarding exceptions to the __exit__ methods of the resources registered using Context.with_resource.

Added an extra function to handle both Context.close and Context.__exit__, called Context._close_with_exception_info that accepts all the parameters that __exit__ and moved the Content.close logic there.

Added relevant tests.

@Jazzinghen
Copy link
Contributor Author

I am not entirely sure what test is failing.

@davidism
Copy link
Member

davidism commented Sep 11, 2025

Failing test seems unrelated. Change looks good.

I'm not entirely certain whether this makes more sense as a bug fix (exit wasn't capturing exception) vs feature (exit now captures exception). It's technically a change in behavior, but the old behavior was incomplete. I think it's fine to target it at stable, just wanted to point out it's borderline.

@Jazzinghen
Copy link
Contributor Author

Failing test seems unrelated. Change looks good.

I'm not entirely certain whether this makes more sense as a bug fix (exit wasn't capturing exception) vs feature (exit now captures exception). It's technically a change in behavior, but the old behavior was incomplete. I think it's fine to target it at stable, just wanted to point out it's borderline.

Ah, yes, this is something I was thinking about as well. The issue was marked as bug, but it's partly a bug, party a "new feature"?
I targeted stable only because the referenced issue was marked as Bug.

@Rowlando13 Rowlando13 changed the base branch from stable to main September 12, 2025 03:08
@Rowlando13
Copy link
Collaborator

@Jazzinghen I moved it to main so I can merge it and have it go out with 8.3.0. There were a lot of changes made to main that are not on stable. Can you check that changes still look right.

@Rowlando13 Rowlando13 added this to the 8.3.0 milestone Sep 12, 2025
@Jazzinghen
Copy link
Contributor Author

@Jazzinghen I moved it to main so I can merge it and have it go out with 8.3.0. There were a lot of changes made to main that are not on stable. Can you check that changes still look right.

I'll rebase it and check it in an hour!

@Jazzinghen Jazzinghen force-pushed the jazzinghen/provide-exception-info-to-resources branch from 5c3def5 to 7c7ec36 Compare September 12, 2025 04:25
@Jazzinghen
Copy link
Contributor Author

Ok, I rebased the branch against main and tested it

@Rowlando13
Copy link
Collaborator

Also just caught it. Please add to changes.rst.

@Jazzinghen
Copy link
Contributor Author

Also just caught it. Please add to changes.rst.

Done!
Is the style correct?

@Rowlando13
Copy link
Collaborator

Yes!

@Rowlando13 Rowlando13 merged commit 36deba8 into pallets:main Sep 12, 2025
10 checks passed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 27, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Exception information isn't passed through to context.with_resource managed resources

3 participants