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.

Sign up for GitHub

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

Tighten annotation of logging.getLevelName #12088

Merged
merged 2 commits into from
Jun 4, 2024
Merged

Tighten annotation of logging.getLevelName #12088

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4651635
Tighten annotation of logging.getLevelName
cjwatson Jun 3, 2024
a9605cf
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Jun 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion stdlib/logging/__init__.pyi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -572,7 +572,14 @@ fatal = critical

def disable(level: int = 50) -> None: ...
def addLevelName(level: int, levelName: str) -> None: ...
def getLevelName(level: _Level) -> Any: ...
@overload
def getLevelName(level: int) -> str: ...

# 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: ...
Comment on lines +578 to +582
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
# 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.)

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

But isn't that true for every use of deprecated? Then why use it at all?

Copy link
Member

Choose a reason for hiding this comment

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

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"

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Member

@AlexWaygood AlexWaygood Jun 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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 to time.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.


if sys.version_info >= (3, 11):
def getLevelNamesMapping() -> dict[str, int]: ...
Expand Down