more type hints for io/formats/format.py by simonjayhawkins · Pull Request #27512 · pandas-dev/pandas (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation13 Commits35 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
another pass to follow-on from #27418
…ent (expression has type "Union[bool, str]", variable has type "None")
… + ("List[str]" and "int")
…ent (expression has type "Optional[int]", variable has type "int")
…ent (expression has type "Optional[int]", variable has type "int")
…ent (expression has type "Optional[int]", variable has type "None")
…fixed_width" has incompatible type "Union[str, int]"; expected "Optional[int]"
…patible type "Union[bool, List[str]]"; expected "Sized"
…int]" for "Union[Any, List[str]]"; expected type "int"
…ent (expression has type "List[str]", variable has type "str")
…ent (expression has type "List[Any]", variable has type "Tuple[Any, ...]")
…fixed_width" has incompatible type "Union[str, int]"; expected "Optional[int]"
…ment (expression has type "Type[Timedelta64Formatter]", variable has type "Type[Datetime64Formatter]")
…ment (expression has type "Union[str, Callable[..., Any], EngFormatter, Type[str], None]", variable has type "Union[Callable[..., Any], partial[Any], None]")
…ment (expression has type "None", variable has type "partial[str]")
…mpatible with supertype "TableFormatter"
Thanks! Haven't looked in detail yet but FYI I think we'll want to get #27424 in as a precursor. There's some failures showing up in there in the io.formats file so may uncover some others here as well since that's a central module getting touched
Haven't checked failures in that PR in detail yet so if you have time / interest in taking a look would be appreciated
| @@ -1207,7 +1262,7 @@ def formatter(value): |
|---|
| return formatter |
| def get_result_as_array(self): |
| def get_result_as_array(self) -> Union[ndarray, List[str]]: |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a Union? why just ndarray?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it depends on self.fixed_width L1303
_trim_zeros_complex L1688 returns List[str]
_trim_zeros_float L1706 returns List[str]
otherwise L1309 returns ndarray
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
grr really? this just sounds wrong, can you cahnge those 2 to return an ndarray(object) if needed; these should all have a consistent signature.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really don't think we should be refactoring at the same time as adding type hints.
it's ok when someone is adding new code or modifying existing code.
but to benefit from the network effect of typing, i want to get this merged and get on with typing call sites and typing the imported functions.
refactoring can be done in parallel.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok sure. its nice that the typing surfaces things like this. pls make an issue (or PR even better!)
| def _format_datetime64(x, tz=None, nat_rep="NaT"): |
|---|
| def _format_datetime64( |
| x: Union[NaTType, Timestamp], |
| tz: Optional[Union[tzfile, tzutc]] = None, |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these are not just strings?
| def __init__(self, values, nat_rep="NaT", box=False, **kwargs): |
|---|
| def __init__( |
| self, |
| values: Union[ndarray, TimedeltaIndex], |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
really?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so due the fact that _format_native_types in pandas/core/indexes/timedeltas.py uses Timedelta64Formatter directly, this should account for the TimedeltaIndex.
| formatter = self.formatter or _get_format_timedelta64( |
|---|
| self.values, nat_rep=self.nat_rep, box=self.box |
| ) |
| fmt_values = np.array([formatter(x) for x in self.values]) |
| return fmt_values |
| def _get_format_timedelta64(values, nat_rep="NaT", box=False): |
| def _get_format_timedelta64( |
| values: Union[ndarray, TimedeltaIndex, TimedeltaArray], |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
really?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
similiarly, _get_format_timedelta64 is imported and used by core\arrays\timedeltas.py and core\indexes\timedeltas.py
thanks @simonjayhawkins if you can address the questions (in this PR) in a followup would be great
thanks @simonjayhawkins if you can address the questions (in this PR) in a followup would be great
no problem. i'm hoping some issues will resolve themselves as more type hints are added to imported functions and the default return type of Any is replaced with more specific types.
so i'm tending towards favoring more liberal use # type: ignore in preference to cast.
However, I think we may start to encounter more and more issues with the untyped _lib.pyx imports. mypy has good understanding of isinstance, but not of the is_* functions.
have spent some time looking at stubbing and overloading, no joy yet. so we may need to add casts after every is_* call.
quintusdias pushed a commit to quintusdias/pandas_dev that referenced this pull request
