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

BaseExceptionGroup.derive should not copy __notes__ #112

Merged
merged 4 commits into from Mar 5, 2024

Conversation

cfbolz
Copy link
Contributor

@cfbolz cfbolz commented Mar 4, 2024

this makes the behaviour follow that of CPython more closely. Instead, copy notes (if present) in the callers of derive. The (modified) test passes now, and it passes on py3.11. It fails before the changes. #111

cfbolz and others added 2 commits March 4, 2024 13:17
this makes the behaviour follow that of CPython more closely. Instead,
copy __notes__ (if present) in the *callers* of derive. The (modified)
test passes now, and it passes on py3.11. It fails before the changes.
@coveralls
Copy link

coveralls commented Mar 4, 2024

Pull Request Test Coverage Report for Build 8155139303

Details

  • 8 of 8 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.02%) to 98.396%

Totals Coverage Status
Change from base Build 7973406898: 0.02%
Covered Lines: 552
Relevant Lines: 561

💛 - Coveralls

@agronholm
Copy link
Owner

Does anything here actually test that derive() doesn't copy __notes__?

@cfbolz
Copy link
Contributor Author

cfbolz commented Mar 5, 2024

Does anything here actually test that derive() doesn't copy __notes__?

The test changes that I did tests it, but very indirectly. So I've now added an explicit test.

Copy link
Owner

@agronholm agronholm left a comment

Choose a reason for hiding this comment

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

LGTM now.

@agronholm agronholm merged commit ee53e9f into agronholm:main Mar 5, 2024
15 checks passed
@agronholm
Copy link
Owner

Thanks!

jakkdl added a commit to jakkdl/exceptiongroup that referenced this pull request Apr 18, 2024
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

3 participants