Tighten annotation of logging.getLevelName #12088
Conversation
To better reflect the implementation's behaviour, python#2730 changed `logging.getLevelName` to accept `int | str` and return `Any` (the latter due to the need to avoid union return types). However, this isn't ideal if you're passing in an `int`, in which case the implementation always returns a `str`. Add overloads for this. This is all arguably a bit unfortunate in light of python#1842 (comment), but I don't want to relitigate that here. I've at least left a comment.
|
According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉 |
|
I'm actually fine with reverting #2730, but this time adding a comment, per @gvanrossum's comment. Alternatively, we could add an overload for both cases, and don't bother with |
|
Maybe I should tag @noseworthy here if we're considering reverting their change. |
|
Fine by me! |
|
A final alternative – and maybe the best – is to add both overloads, but add |
|
It's |
|
I was missing the |
| # The str -> int case is considered a mistake, but retained for backward | ||
| # compatibility. See | ||
| # https://docs.python.org/3/library/logging.html#logging.getLevelName. | ||
| @overload | ||
| def getLevelName(level: str) -> Any: ... |
There was a problem hiding this comment.
| # The str -> int case is considered a mistake, but retained for backward | |
| # compatibility. See | |
| # https://docs.python.org/3/library/logging.html#logging.getLevelName. | |
| @overload | |
| def getLevelName(level: str) -> Any: ... | |
| @overload | |
| @deprecated("The str -> int case is considered a mistake, but retained for backward compatibility.") | |
| def getLevelName(level: str) -> Any: ... |
(Probably needs an import of deprecated from typing_extensions.)
There was a problem hiding this comment.
Surely that would cause a typing error that people would have to either resolve or type: ignore away, in much the same way as if we removed the overload entirely?
There was a problem hiding this comment.
But isn't that true for every use of deprecated? Then why use it at all?
There was a problem hiding this comment.
My understanding was that @deprecated was really intended to let type checkers warn about people using things that had been deprecated at runtime, not so much so that we could invent deprecations at "typing time"
There was a problem hiding this comment.
I still fail to see the difference to other kind of deprecations. Why don't we remove functions that have been deprecated at runtime instead of using @deprecated? Users still have to either # type: ignore those deprecations or resolve them.
There are multiple reasons why deprecations are better than just removing functions:
- "X is deprecated, because ..." is a much better error message than "this function does not exist". Especially, since we can't expect users to look into stubs files to find out the reason for the deprecation.
- Deprecated methods are usually warnings, while non-existing functions are errors – at least in type checkers, linters, IDEs that make this distinction.
- Disabling deprecation warnings globally is usually a better idea than disabling "unknown function" warnings. This is especially useful when e.g. upgrading to a new Python version and you have many or hard to fix deprecations. (I'm looking at you, badly handled
utcnow()deprecation.)
The advantage of using @deprecated (with category=None at runtime) over runtime warnings is that they have much less impact. They are easier to ignore and there's no danger of them unknowingly becoming runtime warnings. As such, I think that we should more liberal in stubs than at runtime with these kinds of warning. Especially in cases like this, where the use of str -> int/str is clearly deprecated in the documentation.
There was a problem hiding this comment.
Yeah, the point you make about the better error message is a very good one; it's true that users often struggle to understand why changes in typeshed suddenly cause their type checking CI job to start failing; you're right that this could help a lot with that.
I'm still a little concerned at the idea of a "typeshed-managed" deprecation. Are we planning on removing the overload eventually? If so, how long a deprecation period are we planning on giving? Should we communicate that deprecation period in the message to the user? How will we remember to remove the deprecated overload when we reach the end of the deprecation period? Should we have a policy around this? Do we need some clear language here to make clear that there's not actually any indication that runtime support for this will ever be removed, it's just a typing-only deprecation?
And the main point I want us to remember is that deprecations can still be really disruptive, but we don't have a good sense of how disruptive at all in our CI at the moment, because mypy doesn't support @deprecated yet, so nothing will show up in mypy_primer
There was a problem hiding this comment.
The possible disruption is a problem, of course. In this particular case, we could check it, by removing the second overload in this PR, check the primer output and then re-add it. But overall I don't consider deprecation warnings to be a huge issue. They are easily disabled globally if they are a problem for a particular project and can be re-enabled when the problem is fixed – something you usually don't want to do with other kinds of typing errors.
There was a problem hiding this comment.
Why don't we remove functions that have been deprecated at runtime instead of using
@deprecated?
To me this depends on how much code is actually using the legacy feature. If basically nobody is using it, even in old codebases, then we might as well pretend that it doesn't exist. This gives users simpler autocompletions and error messages. On the other hand, if the feature is not completely useless and someone is likely still using it, then @deprecated is a great way to communicate that.
A couple examples (both from tkinter because that's the area of typeshed I'm most familiar with):
- Tkinter's
.after()method can be called in a way that makes it equivalent totime.sleep(). This is unacceptable for a GUI application because it freezes the event loop, so nobody uses.after()this way intentionally, even in very old tkinter code. I chose to leave it out entirely rather than mark it@deprecated. - Tkinter variables have
.trace()and.trace_add(). Tcl's documentation says that the underlying Tcl command for.trace()is available "for backwards compatibility", but for obvious reasons, does not state a Python version where it will no longer work. It still makes sense to use@deprecated.
For this PR, I'm tempted to keep the overload as long as logging.getLevelName("WARNING") returns 30 at runtime, but mark it as deprecated. It is confusing, but at least it does something useful, as opposed to freezing the entire app.
To better reflect the implementation's behaviour, python#2730 changed `logging.getLevelName` to accept `int | str` and return `Any` (the latter due to the need to avoid union return types). However, this isn't ideal if you're passing in an `int`, in which case the implementation always returns a `str`. Add overloads for this.
To better reflect the implementation's behaviour,
#2730 changed
logging.getLevelNameto acceptint | strand returnAny(the latter due to the need to avoid union return types). However, this isn't ideal if you're passing in anint, in which case the implementation always returns astr. Add overloads for this.This is all arguably a bit unfortunate in light of #1842 (comment), but I don't want to relitigate that here. I've at least left a comment.