Skip to content

TYP: Add return types to some top-level func #30565

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 6 commits into from
Dec 31, 2019
Merged

TYP: Add return types to some top-level func #30565

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
34bcaa8
Add types to some top-level funcs
topper-123 Dec 30, 2019
4ef2528
Remove NoReturn
topper-123 Dec 30, 2019
635d945
mypy casting
topper-123 Dec 30, 2019
0807828
mypy part II
topper-123 Dec 30, 2019
648c113
mypy add type: ignore[assignment]
topper-123 Dec 31, 2019
5a85a32
Revert "mypy add type: ignore[assignment]"
topper-123 Dec 31, 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
4 changes: 2 additions & 2 deletions pandas/core/indexes/datetimes.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1356,7 +1356,7 @@ def date_range(
name=None,
closed=None,
**kwargs,
):
) -> DatetimeIndex:
"""
Return a fixed frequency DatetimeIndex.

Expand Down Expand Up @@ -1522,7 +1522,7 @@ def bdate_range(
holidays=None,
closed=None,
**kwargs,
):
) -> DatetimeIndex:
"""
Return a fixed frequency DatetimeIndex, with business day as the default
frequency.
Expand Down
6 changes: 3 additions & 3 deletions pandas/core/reshape/merge.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ def merge(
copy: bool = True,
indicator: bool = False,
validate=None,
):
) -> "DataFrame":
op = _MergeOperation(
left,
right,
Expand Down Expand Up @@ -183,7 +183,7 @@ def merge_ordered(
fill_method=None,
suffixes=("_x", "_y"),
how: str = "outer",
):
) -> "DataFrame":
"""
Perform merge with optional filling/interpolation.

Expand Down Expand Up @@ -317,7 +317,7 @@ def merge_asof(
tolerance=None,
allow_exact_matches: bool = True,
direction: str = "backward",
):
) -> "DataFrame":
"""
Perform an asof merge. This is similar to a left-join except that we
match on nearest key rather than equal keys.
Expand Down
9 changes: 5 additions & 4 deletions pandas/core/reshape/pivot.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
from typing import TYPE_CHECKING, Callable, Dict, Tuple, Union
from typing import cast, TYPE_CHECKING, Callable, Dict, Tuple, Union

import numpy as np

Expand Down Expand Up @@ -35,7 +35,7 @@ def pivot_table(
dropna=True,
margins_name="All",
observed=False,
):
) -> "DataFrame":
index = _convert_by(index)
columns = _convert_by(columns)

Expand Down Expand Up @@ -148,7 +148,8 @@ def pivot_table(
table = table.sort_index(axis=1)

if fill_value is not None:
table = table.fillna(value=fill_value, downcast="infer")
filled = table.fillna(value=fill_value, downcast="infer")
table = cast(DataFrame, filled)
Copy link
Member

Choose a reason for hiding this comment

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

I think the preference is for assert instead of cast @jbrockmendel

Suggested change
table = cast(DataFrame, filled)
assert table is not None # needed for mypy

Copy link
Contributor Author

@topper-123 topper-123 Dec 30, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

The error is error: Incompatible types in assignment (expression has type "Optional[DataFrame]", variable has type "DataFrame"). I cant reassign table to a broader type. So that pattern won't work.

Could do

filled = table.fillna(value=fill_value, downcast="infer")
assert filled is not None
table = filled

but that's also ugly. But I agree that asserts are better, so I'll do that one above (unless you got something less ugly :-))

Copy link
Member

Choose a reason for hiding this comment

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

(unless you got something less ugly :-)

we probably should be overloading all methods that have an inplace parameter, but that would require the use of Literal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Literals won’t come until 3.8 is the minimum version, so will be a while. Should we allow assignment to incompatible type in mypy, until 3.8? Seems like a lesser evil?

Copy link
Contributor Author

@topper-123 topper-123 Dec 30, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Ok, should probably use ignore commands in the first line: https://mypy.readthedocs.io/en/latest/error_codes.html#silencing-errors-based-on-error-codes. Then ‘’table =...’’ can be used in the first line and the third line can be dropped.

I’ll update later to use this pattern.

Copy link
Member

Choose a reason for hiding this comment

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

we are using the default for allow_redefinition. It does result in lots of variable renaming and some messy code, but the additional strictness is probably worth it.

Copy link
Member

Choose a reason for hiding this comment

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

Is there any way we could get mypy to understand that without inplace=True isna returns non-None?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not until 3.8 and Literal, AFAIK.

There has been an idea floating around that the inplace parameter should be deprecated everywhere (unless there is a performance benefit to it). That would simplify the return types.

Copy link
Contributor Author

@topper-123 topper-123 Dec 31, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I've changed this section to use # type: ignore directive.

EDIT: didn't work, got a Flake8 F821 error (undefined name). I've reverted the commit back to the 3-line version.

Copy link
Member

@simonjayhawkins simonjayhawkins Dec 31, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

it's a good idea, we need to move the adoption of error codes forward. see #29197 could add # noqa for now.


if margins:
if dropna:
Expand Down Expand Up @@ -426,7 +427,7 @@ def _convert_by(by):

@Substitution("\ndata : DataFrame")
@Appender(_shared_docs["pivot"], indents=1)
def pivot(data: "DataFrame", index=None, columns=None, values=None):
def pivot(data: "DataFrame", index=None, columns=None, values=None) -> "DataFrame":
if values is None:
cols = [columns] if index is None else [index, columns]
append = index is None
Expand Down
2 changes: 1 addition & 1 deletion pandas/io/json/_normalize.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ def _json_normalize(
errors: Optional[str] = "raise",
sep: str = ".",
max_level: Optional[int] = None,
):
) -> "DataFrame":
"""
Normalize semi-structured JSON data into a flat table.

Expand Down