-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
[MNT] Typing: Use Literal for set_loglevel #30302
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
base: main
Are you sure you want to change the base?
[MNT] Typing: Use Literal for set_loglevel #30302
Conversation
Hi all 👋, I initially attempted to address multiple points from this issue in a single PR, but I realized that it introduced unnecessary complexity, especially for someone like me who is still learning and new to contributing at this scale. To manage things better and ensure clarity in both review and implementation, I’ve now shifted to handling one fix at a time. I’ve started with set_loglevel as it seemed like a simpler, self-contained change to begin with. This approach will also help me improve gradually and avoid making mistakes across unrelated parts. Thank you so much for your patience and guidance, it’s really encouraging for a beginner like me 🙏🙂 |
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.
Ok, let's take this as is. And handle the question of aligning with stdlib as a follow-up topic.
Yes @timhoffm, you’re right. I’ve implemented this by normalising the input to lowercase within set_loglevel(), so it aligns with the stdlib behaviour. You think that will work? Let me know if any further changes are required. |
@@ -52,7 +55,8 @@ __bibtex__: str | |||
__version__: str | |||
__version_info__: _VersionInfo | |||
|
|||
def set_loglevel(level: str) -> None: ... | |||
def set_loglevel(level: LogLevel | str) -> None: | |||
logging.getLogger("matplotlib").setLevel(level.lower()) |
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.
What are you doing here? You can't implement in a stub file.
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.
Oh , sorry. What do u suggest ? @timhoffm 😅
How about expanding the literal which includes the uppercase as well.
LogLevel: TypeAlias = Literal[
"notset", "NOTSET",
"debug", "DEBUG",
"info", "INFO",
"warning", "WARNING",
"error", "ERROR",
"critical", "CRITICAL"
]
What do u think?
This PR addresses part of #30257 by improving type safety and IDE support for set_loglevel.
Changes in this PR:
This improves discoverability and static type checking for valid logging levels like "info", "debug", "error", etc.
Related work not included here
MarkerType updates were implemented by @ZPyrolink in #30261
Event type literals for connect methods were contributed by @ZPyrolink in #30275