Skip to content

Add warnings.catch_warning type hints with Literal. #3464

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 1 commit into from
Nov 14, 2019

Conversation

mslapek
Copy link
Contributor

@mslapek mslapek commented Nov 14, 2019

Fixes #3463

Copy link
Collaborator

@srittau srittau left a comment

Choose a reason for hiding this comment

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

Thanks!

@srittau srittau merged commit 34d68ab into python:master Nov 14, 2019
@bluetech
Copy link
Contributor

bluetech commented Nov 14, 2019

This is a nice change, thanks.

However, I wonder if there is a better way to fix the issue. This PR changes catch_warning from a class to a function which returns typing.ContextManager. This has two downsides:

  1. It breaks the usual rule of "accept abstract, return concrete". Returning an abstract interface is strictly weaker than returning a concrete type which implements this interface. It does make sense if the implementation wants to keep its options open, but that's not the case here.

  2. The docs document catch_warnings as a class. Changing it to a function means (I think) that the type checker will no longer allow to subclass it, or check isinstance of it, return it from wrapper functions, etc.

Maybe @srittau knows how to keep it a class? Maybe it's possible to @overload __init__ and inject a generic param to self in some way? Dunno.

@srittau
Copy link
Collaborator

srittau commented Nov 14, 2019

I agree that keeping it as a class would be preferable. From what I understand overloaded init method that allow to narrow the type of a typevar is something that is currently being worked on in mypy. So we can probably improve these hints soon.

@blueyed
Copy link
Contributor

blueyed commented Nov 22, 2019

As pointed out already this fails now:

import warnings


class WarningsRecorder(warnings.catch_warnings):
    pass
t.py:4: error: Function "warnings.catch_warnings" is not valid as a type
t.py:4: note: Perhaps you need "Callable[...]" or a callback protocol?
t.py:4: error: Invalid base class "warnings.catch_warnings"
Found 2 errors in 1 file (checked 1 source file)

Are there plans to fix / allow this still?

@JelleZijlstra
Copy link
Member

It looks like we should revert to an implementation that uses a class and overloads __new__. Contributions would be welcome.

@mslapek
Copy link
Contributor Author

mslapek commented Nov 25, 2019

I've performed modification in #3499 to "eat the cake and have it too".

@JelleZijlstra Thanks for tip with __new__ overloading.

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.

Output type of warnings.catch_warnings with record=True
5 participants