Skip to content

Attempt to fix multierror typing #2787

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

Merged
merged 5 commits into from
Sep 3, 2023
Merged

Conversation

CoolCat467
Copy link
Member

At the moment, this is only ignoring generics not matching properly in the subclass. If anyone has any better ideas on how to handle this, feel free to try it out.

@codecov
Copy link

codecov bot commented Sep 3, 2023

Codecov Report

Merging #2787 (8f2d55f) into master (4d19399) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2787   +/-   ##
=======================================
  Coverage   98.94%   98.94%           
=======================================
  Files         113      113           
  Lines       16919    16919           
  Branches     3050     3050           
=======================================
  Hits        16741    16741           
  Misses        123      123           
  Partials       55       55           
Files Changed Coverage Δ
trio/_core/_multierror.py 100.00% <ø> (ø)

@CoolCat467 CoolCat467 changed the title Attept to fix multierror typing Attempt to fix multierror typing Sep 3, 2023
@TeamSpen210
Copy link
Contributor

I think the correct solution is to change MultiError/NonBaseMultiError to also be generic, and propagate that throughout the code.

@A5rocks
Copy link
Contributor

A5rocks commented Sep 3, 2023

I tried and it was annoying (some branch in my fork...)

@CoolCat467
Copy link
Member Author

I think the correct solution is to change MultiError/NonBaseMultiError to also be generic, and propagate that throughout the code.

That sounds like a good idea, I'll start trying to do that now.

@TeamSpen210
Copy link
Contributor

It probably is fine if we add more ignores here, since it's deprecated anyway. Users should only be annotating/interacting with ExceptionGroup.

@CoolCat467
Copy link
Member Author

It's looking like it might not be possible to make NonBaseMultiError generic, because ExceptionGroup which we are subclassing itself needs to be using the generic we are defining

@CoolCat467
Copy link
Member Author

I've given up attempting to make NonBaseMultiError generic, I don't think it's easily possible without doing some nasty things at runtime with monkeypatching

Copy link
Contributor

@A5rocks A5rocks left a comment

Choose a reason for hiding this comment

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

Assuming this passes type checking and that the weird previous changes showing in the diff thing is fixed, this looks good to me.

@A5rocks
Copy link
Contributor

A5rocks commented Sep 3, 2023

Actually the merge looks quite messed up -- maybe reset to a clean state, rebase onto master, and copy over the type ignore deletion + new type ignore?

@CoolCat467 CoolCat467 force-pushed the fix-multierror-typing branch from 57dcb3e to 721bfca Compare September 3, 2023 05:54
@CoolCat467 CoolCat467 marked this pull request as ready for review September 3, 2023 05:57
Copy link
Contributor

@A5rocks A5rocks left a comment

Choose a reason for hiding this comment

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

Looks good

@richardsheridan richardsheridan merged commit 2cd5a6f into master Sep 3, 2023
@richardsheridan richardsheridan deleted the fix-multierror-typing branch September 3, 2023 14:57
@A5rocks A5rocks added the typing Adding static types to trio's interface label Sep 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
typing Adding static types to trio's interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

typing error on master in _core/_multierror since exceptiongroup==1.1.3
4 participants