Skip to content

PERF: lazify type-check import #28342

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Sep 9, 2019
+2 −5
Merged

PERF: lazify type-check import #28342

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
11840f1
PERF: lazify type-check import
jbrockmendel Sep 8, 2019
a34564f
change to tzinfo
jbrockmendel Sep 8, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions pandas/io/formats/format.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,6 @@
)
from unicodedata import east_asian_width

from dateutil.tz.tz import tzutc
from dateutil.zoneinfo import tzfile
import numpy as np

from pandas._config.config import get_option, set_option
Expand Down Expand Up @@ -75,6 +73,9 @@
from pandas.io.formats.printing import adjoin, justify, pprint_thing

if TYPE_CHECKING:
from dateutil.tz.tz import tzutc # noqa:F401
from dateutil.zoneinfo import tzfile # noqa:F401

from pandas import Series, DataFrame, Categorical

formatters_type = Union[
Expand Down Expand Up @@ -1553,7 +1554,7 @@ def _is_dates_only(

def _format_datetime64(
x: Union[NaTType, Timestamp],
tz: Optional[Union[tzfile, tzutc]] = None,
tz: Optional[Union["tzfile", "tzutc"]] = None,
Copy link
Member Author

Choose a reason for hiding this comment

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

its not obvious to me why these only care about dateutil tzinfos and not e.g. stdlib or pytz versions. @simonjayhawkins any idea?

Copy link
Member

Choose a reason for hiding this comment

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

These were the types seen by MonkeyType. I think they are resolving to Any.

i've since abandoned using MonkeyType for a couple of reasons..

MonkeyType uses nominal types and in many cases these resolve to Any due to unfollowed imports.

MonkeyType only adds nominal types whereas we'd probably prefer structural types.

feel free to change or remove. The order of typing priority from high to low should probably match the order used in isort. so I consider these to be low priority type hints.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK. I would be shocked if these weren't supposed to be tzinfo, will look more closely and update.

Copy link
Member

Choose a reason for hiding this comment

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

yes, but if tzinfo is an unfollowed import, it'll just resolve to Any.

Copy link
Member Author

Choose a reason for hiding this comment

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

what determines whether something is an unfollowed import? tzinfo is stdlib, seems like mypy should know what it is

Copy link
Member

@simonjayhawkins simonjayhawkins Sep 8, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

These were the types seen by MonkeyType. I think they are resolving to Any.

Revealed type for tz is Union[Any, dateutil.tz.tz.tzutc, None]

so it is only the dateutil.zoneinfo.tzfile that is unfollowed.

its not obvious to me why these only care about dateutil tzinfos and not e.g. stdlib or pytz versions.

pytz.tzinfo.DstTzInfo is also unknown to mypy.

tzinfo is stdlib, seems like mypy should know what it is

since all three (dateutil.zoneinfo.tzfile, dateutil.tz.tz.tzutc and pytz.tzinfo.DstTzInfo) inherit from datetime.tzinfo, and datetime.tzinfo is known to mypy through the stdlib, then it probably does make sense to use datetime.tzinfo here.

However, within the function, tz is only used in Timestamp(x).tz_convert(tz) and Timestamp(x).tz_localize(tz) and Timestamp resolves to any due to an unfollowed import. (needs stub #28195) so no actual type checking is being performed.

so probably best just to delete the type hint for now.

nat_rep: str = "NaT",
) -> str:
if x is None or (is_scalar(x) and isna(x)):
Expand Down