Skip to content

CLN: Optional[Hashable] in dict type hints #40534

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 2 commits into from
Mar 23, 2021

Conversation

topper-123
Copy link
Contributor

@topper-123 topper-123 commented Mar 20, 2021

Minor clean-up.

>>> from typing import Hashable
>>> isinstance(None, Hashable)
True

In old versions of mypy, mypy had a bug and didn't see this, but is has been fixed now.

There are many instances of Optional[Hashable] instead of just Hashable in the code base, other than in these dicts. I've assumed those should be left as-is for readability and not because it's strictly needed.

@jbrockmendel
Copy link
Member

for a while we had an alias Label=Optional[Hashable], but i think weve been replacing that with just Hashable. if im right, then you can just go town on all occurrences. cc @simonjayhawkins ?

@simonjayhawkins
Copy link
Member

for a while we had an alias Label=Optional[Hashable]

that was removed in #39107, although looking back, I didn't do the follow-up

And so I also missed the cases of Optional[Hashable] in the codebase that maybe should have been Label previously, but there was some discussion on when to use Label #32371 (comment) and it looks like these cases fell through the cracks.

Thanks @topper-123 generally lgtm. I don't think necessary to change the comments but nbd

@simonjayhawkins simonjayhawkins added the Typing type annotations, mypy/pyright type checking label Mar 22, 2021
@simonjayhawkins simonjayhawkins added this to the 1.3 milestone Mar 22, 2021
@simonjayhawkins
Copy link
Member

simonjayhawkins commented Mar 22, 2021

if im right, then you can just go town on all occurrences. cc @simonjayhawkins ?

maybe in some cases helps readablity, we have no_implicit_optional = True in setup.cfg for that reason.

@topper-123
Copy link
Contributor Author

There are many case for function/method parameters like this:

def func_name(kw: Optional[Hashable] = None, kw_2) -> ...:
    ...

In such cases the Optional is not necessary wrt. mypy, because None is hashable. But I think it has been done this way for readability as Optional[Hashable] emphasizes that None is different than other hashables. In the dict cases in this PR, that isn't the case and in these cases None isn't special.

I don't have a very strong opinions, but somewhat prefer Optional[Hashable] in cases where None has a different meaning than other hashables to visually show the special status of None.

@WillAyd WillAyd merged commit 05a0e98 into pandas-dev:master Mar 23, 2021
@WillAyd
Copy link
Member

WillAyd commented Mar 23, 2021

Thanks @topper-123

@topper-123 topper-123 deleted the cln_optional_hashable branch March 23, 2021 10:10
JulianWgs pushed a commit to JulianWgs/pandas that referenced this pull request Jul 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Typing type annotations, mypy/pyright type checking
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants