-
-
Notifications
You must be signed in to change notification settings - Fork 143
Permit covariance of key type in read_csv converters argument #450
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
tests/test_frame.py
Outdated
@@ -1278,6 +1279,43 @@ def test_read_csv() -> None: | |||
pd.DataFrame, | |||
) | |||
|
|||
# Allow a variety of dict types for the converters parameter | |||
converters1 = {"A": lambda x: str, "B": lambda x: str} |
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.
Originally I wanted this dictionary to be {"A": str, "B": str}
. But the type of str
is Type[str]
, which apparently isn't compatible with Callable[[str], Any]
. Any idea if this is fixable?
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.
Might be an issue with using a lambda
function, which is untyped. So if you did
def convert_to_str(a: object) -> str:
return str(a)
and then
converters1 = {"A":convert_to_str, "B": convert_to_str}
that might work
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, the current version works fine because it's actually a Callable
. What isn't working is passing Type[str]
directly. That makes the dict a Dict[str, Type[str]]
. That's what I was wondering about.
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 guess the type checkers know that str
could be either a type or a callable.
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.
Try avoiding the lambda
if you can. (or at least add tests without it)
pandas-stubs/io/parsers/readers.pyi
Outdated
converters: dict[int | str, Callable[[str], Any]] | None = ..., | ||
converters: dict[int | str, Callable[[str], Any]] | ||
| dict[int, Callable[[str], Any]] | ||
| dict[str, Callable[[str], Any]] |
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.
Can you try Mapping[int | str, Callable[[str], Any]]
instead of the union?
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 tried that first. But it seems Mapping
isn't covariant over the key, only the value: python/typing#445
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.
But mapping may work for that the Type[str]
issue since that is the value...will try that
I also wonder if creating a covariant TypeVar
and then a Mapping[T_co, ...]
will work...I will also try that.
Ok, the version I just pushed should fix all of that, and also fix the other parameters too - This is something that is probably going to come up over and over again throughout this code base. I think as a rule of thumb all |
…ovariance, and add tests for na_values and parse_dates
I think we have to look at all of them and see. Probably |
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.
thanks @gandhis1
assert_type()
to assert the type of any return valueIf it accepts a
Dict[int | str, ...]
it should accept aDict
of either key type.