-
-
Notifications
You must be signed in to change notification settings - Fork 19.4k
TYP: Mypy workaround for NoDefault #47045
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
Conversation
|
Something similar could also be done for |
pandas/core/indexes/datetimes.py
Outdated
| weekmask=None, | ||
| holidays=None, | ||
| closed: lib.NoDefault = lib.no_default, | ||
| closed: Literal["left", "right"] | lib.NoDefault | None = lib.no_default, |
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.
all the closed annotations seem unrelated but they are a result of this line
| # Note: no_default is exported to the public API in pandas.api.extensions | ||
| no_default = NoDefault.no_default # Sentinel indicating the default value. | ||
| no_default = _NoDefault.no_default # Sentinel indicating the default value. | ||
| NoDefault = Literal[_NoDefault.no_default] |
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.
Instead of defining NoDefault in lib.pyx and lib.pyi it could be defined in _typing.py (would require changing a lot of import statements)
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.
its actually better where it is as we avoid circular deps this way
Dr-Irv
left a comment
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.
Two comments:
- There is a PEP for sentinel values: https://peps.python.org/pep-0661/ so if that PEP is approved, maybe we should use a pattern here that makes it easy for us to switch to using that feature if it gets approved.
- There is a lot of usage of
Literal["left", "right"]in this PR, so maybe we should create a type for that in_typing.pyand use that instead.
Once we can use this PEP (might take a long time, especially without typing_extensions), we simply need to change the two lines that define
Will do |
OK, I see the path forward. However, while |
Dr-Irv
left a comment
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.
I'm fine with this, although I think others should approve as well.
jreback
left a comment
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.
lgtm
|
Thanks @twoertwein |
* TYP: NoDefault * ix mypy issues; re-write isinstance(..., NoDefault) * remove two more casts * ENH: DatetimeArray fields support non-nano (pandas-dev#47044) * DEPR: groupby numeric_only default (pandas-dev#47025) * DOC: Clarify decay argument validation in ewm when times is provided (pandas-dev#47026) * DOC: Fix some typos in pandas/. (pandas-dev#47022) * remove two more casts * avoid cast-like annotation * left/right * cannot use | Co-authored-by: jbrockmendel <jbrockmendel@gmail.com> Co-authored-by: Richard Shadrach <45562402+rhshadrach@users.noreply.github.com> Co-authored-by: Matthew Roeschke <emailformattr@gmail.com> Co-authored-by: Shuangchi He <34329208+Yulv-git@users.noreply.github.com>
This uses the mypy-workaround from https://github.com/python/typeshed/pull/7127/files for
NoDefault(works also for pyright).no_defaultis public butNoDefaultis luckily not public, so it should be fine to do the following:NoDefaultto_NoDefault(implementation and typing)NoDefaultasNoDefault = Literal[_NoDefault.no_default](implementation and typing;isinstancechecks need to be re-written to useis no_defaultbut all other type annotations can stay the same)no_default"remains"no_default = _NoDefault.no_default(just replacingNoDefaultwith_NoDefault)@simonjayhawkins @jreback @Dr-Irv
edit:
Some context: The following will work without mypy errors afterwards: