TYPING: more type hints for io.common

[simonjayhawkins]

No description provided.

[jreback]

https://stackoverflow.com/questions/37553551/3-5-type-hinting-and-method-overloading

is there a reason we need overload here?

[simonjayhawkins]

we could be less strict and use casts or ignores instead where necessary

[jreback]

this seems to be hiding a bug then, these signatures should all be the same no? why do we need different signatures?

[simonjayhawkins]

the signatures are the same. the types are different.

buf passed through unchanged (as typevar)

Path converted to string

so overload is so that if a Path is given a string is returned.

[jreback]

I see so this is essentially doing a Union of 2 TypeVars then. ok, can you give a couple of comments before the use of overload then to explain what is happening. my hesitation is that the next person to look at this will not understand w/o some hints.

[simonjayhawkins]

yep.

[simonjayhawkins]

from typing import overload


class Foo:
    pass


class Bar:
    pass


@overload
def foo(x: Foo, y: Bar) -> str:
    ...


@overload
def foo(x: Bar, y: Foo) -> int:
    ...


def foo(x, y):
    if isinstance(x, Foo) and isinstance(y, Bar):
        return "foo"
    elif isinstance(x, Bar) and isinstance(y, Foo):
        return 42
    else:
        raise TypeError(x, y)


foo(Foo(), Bar())
foo(Bar(), Foo())
foo(Foo(), Foo())
foo(Bar(), Bar())

$ mypy test.py
test.py:33: error: No overload variant of "foo" matches argument types "Foo", "Foo"
test.py:33: note: Possible overload variants:
test.py:33: note:     def foo(x: Foo, y: Bar) -> str
test.py:33: note:     def foo(x: Bar, y: Foo) -> int
test.py:34: error: No overload variant of "foo" matches argument types "Bar", "Bar"
test.py:34: note: Possible overload variants:
test.py:34: note:     def foo(x: Foo, y: Bar) -> str
test.py:34: note:     def foo(x: Bar, y: Foo) -> int

[simonjayhawkins]

I see so this is essentially doing a Union of 2 TypeVars then. ok, can you give a couple of comments before the use of overload then to explain what is happening. my hesitation is that the next person to look at this will not understand w/o some hints.

@jreback I imagine we could be using overload quite a bit. I'll add comments if you think necessary, but it may end up adding quite a bit of duplication with the docstrings over time.

[jreback]

maybe add a small section in the contributing docs then of why / how we are using it?

[simonjayhawkins]

Contributing Guide for Type Hints #27050 is not yet merged. added comments for now using comments from https://mypy.readthedocs.io/en/latest/more_types.html#function-overloading as boilerplate.

[jreback]

why is this here and not correctly coming from get_filepath_or_buffer?

[simonjayhawkins]

get_filepath_or_buffer can return a str.

as stated before, my preference would be to always add #type:ignore rather than change code unnecessarily. The code is currently type safe due to the try/except block, but mypy doesn't see that.

without the isinstance check, pandas\io\parsers.py:463: error: Item "str" of "Union[str, IO[Any]]" has no attribute "close"

[simonjayhawkins]

The code is currently type safe due to the try/except block, but mypy doesn't see that.

sorry i'm wrong here.

it would be type safe if we used except (AttributeError, ValueError) but mypy would still report the same error and would need either an ignore or the isinstance check to pass.

at the moment should_close is never True when fp_or_buf is a string. but then again the purpose of type checking is to prevent bugs that may be added in the future.

maybe get_filepath_or_buffer should be converted to a context manager and handle the closing.

[WillAyd]

Hmm somewhat confused by this comment. So does the overload fall back to maintaining the "generic" nature of items in the TypeVar that haven't been overloaded? I think that would be desirable

[simonjayhawkins]

The comment is copy and pasted from https://mypy.readthedocs.io/en/latest/more_types.html#function-overloading

When adding the overloads I removed the type annotations from the actual implementation to avoid duplication.

I guess this was the wrong thing to do. will add them back.