Skip to content

Conversation

phofl
Copy link
Member

@phofl phofl commented Oct 3, 2021

cc @simonjayhawkins Could you have a look and give some feedback if this is ok so?

@jreback jreback added the Typing type annotations, mypy/pyright type checking label Oct 3, 2021
if not is_list_like(columns) or isinstance(columns, tuple):
columns = [columns]

assert isinstance(columns, abc.Iterable)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if adding an assert for mypy, append an inline comment # for mypy since we can't catch these when no longer necessary (i.e. if TypeGuard can narrow the types from the is_list_like condition.)

Alternatively, use a cast. we use cast elsewhere after is_* calls and we can detect when not needed with warn_redundant_casts mypy setting.

maybe Iterable from typing instead of collections but typing.Iterable is deprecated https://docs.python.org/3/library/typing.html#typing.Iterable

so okay to use typing.Iterable for consistency and also okay to keep the import from collections.

we will need to do a sweep at some point to migrate the deprecated uses.

Copy link
Member Author

@phofl phofl Oct 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thx this helps a lot. I have replaced the assert with a cast. Is cast preferred in general?

Should we prefer the abc imports in general if the typing equivalent is deprecated?

Copy link
Member

@jbrockmendel jbrockmendel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jreback jreback added this to the 1.4 milestone Oct 19, 2021
@jreback jreback merged commit 1bcb841 into pandas-dev:master Oct 19, 2021
@phofl phofl deleted the typ_insert branch October 20, 2021 19:49
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