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

Add wurlitzer stubs #11459

Merged
merged 22 commits into from
Mar 9, 2024
Merged

Add wurlitzer stubs #11459

merged 22 commits into from
Mar 9, 2024

Conversation

hoel-bagard
Copy link
Contributor

This PR adds stubs for the wurlitzer package.

This comment has been minimized.

2 similar comments

This comment has been minimized.

This comment has been minimized.

@hoel-bagard hoel-bagard marked this pull request as ready for review February 21, 2024 12:44
@hoel-bagard hoel-bagard force-pushed the hoel/add_wurlitzer_package branch from 5e9fca9 to 21be2e5 Compare February 21, 2024 12:57
Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

Mostly looks good, though I haven't looked in-depth at the stderr/stdout annotations. Usually we prefer to defined protocols for that kind of thing rather than mandating that an object has to be an instance of io.IOBase. Usually that isn't necessary and duck-typing will do.

stubs/wurlitzer/wurlitzer/__init__.pyi Outdated Show resolved Hide resolved
stubs/wurlitzer/wurlitzer/__init__.pyi Outdated Show resolved Hide resolved
stubs/wurlitzer/wurlitzer/__init__.pyi Outdated Show resolved Hide resolved
stubs/wurlitzer/wurlitzer/__init__.pyi Outdated Show resolved Hide resolved

This comment has been minimized.

@hoel-bagard
Copy link
Contributor Author

hoel-bagard commented Feb 21, 2024

Thanks for the (very fast 😮 ) review, I'll look into using protocols.

Edit: looks like some overloads will be required to get precise return types, I'm passing this PR back into draft mode for now.

This comment has been minimized.

This comment has been minimized.

@hoel-bagard hoel-bagard marked this pull request as draft February 21, 2024 13:22
@hoel-bagard hoel-bagard force-pushed the hoel/add_wurlitzer_package branch from d577d5b to 80f410c Compare February 21, 2024 14:12
@hoel-bagard
Copy link
Contributor Author

@AlexWaygood Do you know what in the overloads is causing the CI to fail ?

This comment has been minimized.

@Akuli
Copy link
Collaborator

Akuli commented Feb 21, 2024

The error looks like a bug in stubtest, but you can probably work around it by not using overloaded context managers. Instead of @contextmanager together with -> Iterator[Foo], you could try -> AbstractContextManager[Foo] or -> _GeneratorContextManager[Foo]. You need to import from contextlib.

@AlexWaygood
Copy link
Member

The error looks like a bug in stubtest, but you can probably work around it by not using overloaded context managers. Instead of @contextmanager together with -> Iterator[Foo], you could try -> AbstractContextManager[Foo] or -> _GeneratorContextManager[Foo]. You need to import from contextlib.

Yeah, @Akuli is right. Stubtest in general isn't great at dealing with several decorators on a single function -- see python/mypy#14950 (which is now fixed, but was only fixed by special-casing @final).

This comment has been minimized.

@hoel-bagard hoel-bagard marked this pull request as ready for review February 21, 2024 23:22

This comment has been minimized.

@hoel-bagard
Copy link
Contributor Author

@AlexWaygood If you have time to review the changes.

Copy link
Collaborator

@Akuli Akuli left a comment

Choose a reason for hiding this comment

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

Thanks for working on this! Typing is hard :)

stubs/wurlitzer/wurlitzer.pyi Outdated Show resolved Hide resolved
stubs/wurlitzer/wurlitzer.pyi Outdated Show resolved Hide resolved
stubs/wurlitzer/wurlitzer.pyi Outdated Show resolved Hide resolved
stubs/wurlitzer/wurlitzer.pyi Outdated Show resolved Hide resolved
stubs/wurlitzer/wurlitzer.pyi Outdated Show resolved Hide resolved
stubs/wurlitzer/wurlitzer.pyi Outdated Show resolved Hide resolved
stubs/wurlitzer/wurlitzer.pyi Outdated Show resolved Hide resolved
stubs/wurlitzer/wurlitzer.pyi Outdated Show resolved Hide resolved
@hoel-bagard
Copy link
Contributor Author

@Akuli Thanks for the improvements/fixes!

Copy link
Contributor

github-actions bot commented Mar 9, 2024

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

Copy link
Collaborator

@Akuli Akuli left a comment

Choose a reason for hiding this comment

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

Thanks!

@Akuli Akuli merged commit beba8b1 into python:main Mar 9, 2024
43 checks passed
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.

3 participants