-
-
Notifications
You must be signed in to change notification settings - Fork 17
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 stubtest, fix incorrect stubs #73
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One comment below, but this looks like a nice improvement over the status quo to me!
(I'd love for someone to look through the allowlist and decide whether we need any further fixes, but IMO that should not block this PR)
def __init__( | ||
self, exceptions: Sequence[BaseException], *, _collapse: bool = ... | ||
): ... | ||
def __new__( | ||
cls: T, exceptions: Sequence[BaseException], *, _collapse: bool = ... | ||
) -> T: ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably better to not expose this param in stubs, since it's got a leading underscore and use of MultiError
is deprecated anyway!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, it is just a stub - and not an official API/documentation. If there weren't a stub file, type checkers would see the parameters, and in error messages and the like you could see references to it. So I think I'm leaning towards keeping it.
I think having fewer lines in the allowlist also is a good thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant "have a stub for MultiError, which omits the _collapse
parameter", I think we agree on the first part but I miscommunicated about the second?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah no that's how I interpreted you 😅 So the miscommunication must be on my part! :D
I think adding trio.MultiError.__init__
and trio.MultiError.__new__
to allowlist.txt
isn't great. Another reason for keeping the stubs technically correct is to ease a transition away from having separate stub files at all, and if exposing _collapse
is bad for some reason that should be addressed in trio
- rather than hidden by having an "incorrect" type for the function in the stub.
Noticed due to
open_nursery
missing thestrict_exception_types
argument, but various other issues raised by stubtest also addressed to the best of my ability. If any of them are problematic, it's easy to chuck them into the allowlist instead.It's very possible that a bunch of the missing stubs in the allowlist should be added to the stubs.
I have no clue why
ci.sh
creates an empty directory and symlinks inCHECK_TYPING
, but whatever the reason I did the same forCHECK_STUBS
.