Skip to content

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