Allow literals as kwargs dict keys#20416
Conversation
hauntsaninja
changed the title
|
Diff from mypy_primer, showing the effect of this PR on open source code: pandas (https://github.com/pandas-dev/pandas)
+ pandas/core/generic.py:4771: error: Unused "type: ignore[misc]" comment [unused-ignore]
+ pandas/core/generic.py:4842: error: Unused "type: ignore[misc]" comment [unused-ignore]
+ pandas/core/generic.py:5610: error: Unused "type: ignore" comment [unused-ignore]
rotki (https://github.com/rotki/rotki)
+ rotkehlchen/exchanges/binance.py:355: error: Unused "type: ignore" comment [unused-ignore]
xarray (https://github.com/pydata/xarray)
- xarray/core/common.py:460: error: Keywords must be strings [misc]
+ xarray/core/common.py:460: error: Argument after ** must have string keys [arg-type]
- xarray/core/dataset.py:5204: error: Keywords must be strings [misc]
+ xarray/core/dataset.py:5204: error: Argument after ** must have string keys [arg-type]
|
| return ( | ||
| is_subtype( | ||
| ( | ||
| # This is a little ad hoc, ideally we would have a map_instance_to_supertype |
There was a problem hiding this comment.
Imo this whole approach is technically incorrect. SupportsKeysAndGetItem is invariant in key type, so prescribing str will cause errors if a dict with literal keys is given
| [case testLiteralKwargs] | ||
| from typing import Any, Literal | ||
| kw: dict[Literal["a", "b"], Any] | ||
| def func(a, b): ... | ||
| func(**kw) | ||
|
|
||
| badkw: dict[Literal["one", 1], Any] | ||
| func(**badkw) # E: Argument after ** must have string keys | ||
| [builtins fixtures/dict.pyi] |
There was a problem hiding this comment.
This modified test produces false-positive for good_kw:
# [case testLiteralKwargs]
from typing import Literal
def func(a: int, b: int) -> None: ...
class A: pass
good_kw: dict[Literal["a", "b"], int]
bad_kw: dict[Literal["one", 1], int]
func(**good_kw)
func(**bad_kw) # E: Argument after ** must have string keys
# [builtins fixtures/dict.pyi]There was a problem hiding this comment.
Hm, this PR does handle correctly handle that test case. The patch is ad hoc though, so e.g. it would fail if you changed it to Mapping
There was a problem hiding this comment.
You're right, I think tested with the wrong commit
There was a problem hiding this comment.
I did make the proposed change into a helper function: #20419
|
I've added a code that fixes the I could move this into a separate PR, and maybe it's worth making that procedure into a helper function? |
3 participants
Fixes #10023 , fixes #13674 , closes #10237