Skip to content
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 and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TYP: Type annotations in pandas/io/formats/style.py #30403

Merged
merged 20 commits into from
Jan 20, 2020

Conversation

ShaharNaveh
Copy link
Member

@ShaharNaveh ShaharNaveh commented Dec 22, 2019

  • closes #xxxx
  • tests added / passed
  • passes black pandas
  • passes git diff upstream/master -u -- "*.py" | flake8 --diff
  • whatsnew entry

@ShaharNaveh ShaharNaveh marked this pull request as ready for review December 22, 2019 13:58
@@ -1459,7 +1487,7 @@ def pipe(self, func, *args, **kwargs):
return com.pipe(self, func, *args, **kwargs)


def _is_visible(idx_row, idx_col, lengths):
def _is_visible(idx_row, idx_col, lengths) -> bool:
Copy link
Member

Choose a reason for hiding this comment

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

can the inputs by typed?

Copy link
Member Author

Choose a reason for hiding this comment

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

The problem is that mypy is giving false positives, I typed the 'row' as str and the 'col' as int and it went through with no errors.

@@ -879,7 +891,7 @@ def set_caption(self, caption):
self.caption = caption
return self

def set_table_styles(self, table_styles):
def set_table_styles(self, table_styles: List) -> "Styler":
Copy link
Member

Choose a reason for hiding this comment

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

can we say anything about what goes in the List?

Copy link
Member

Choose a reason for hiding this comment

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

(same question for other occurrences of List above)

Copy link
Member Author

@ShaharNaveh ShaharNaveh Dec 22, 2019

Choose a reason for hiding this comment

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

The doc type says it's should be List
but the description says it should be Dict.

Or I misunderstood (more likely).

Each individual table_style should be a dictionary with 'selector' and 'props' keys.

Copy link
Member Author

Choose a reason for hiding this comment

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

When trying to annotate it as List[str]

mypy gives this error:

pandas/io/formats/style.py:861: error: Argument 1 to "extend" of "list" has incompatible type "List[str]"; expected "Iterable[Tuple[Callable[..., Any], Tuple[Any, ...], Dict[Any, Any]]]"
Found 1 error in 1 file (checked 794 source files)

expected "Iterable[Tuple[Callable[..., Any], Tuple[Any, ...], Dict[Any, Any]]]"

Maybe I should leave it unannotated, or leave it as List, or put Any just in case?

@@ -849,7 +861,7 @@ def use(self, styles):
self._todo.extend(styles)
return self

def set_uuid(self, uuid):
def set_uuid(self, uuid: str) -> "Styler":
Copy link
Member

Choose a reason for hiding this comment

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

i dont actually use this code, is the uuid here going to be specifically a hex str? if so, we should look into whether there is a more specific annotatiton for that

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 mechanism for this would be NewType, https://mypy.readthedocs.io/en/latest/more_types.html#newtypes

Copy link
Member Author

Choose a reason for hiding this comment

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

@simonjayhawkins Don't you think that's a bit of an overkill?
By overkill I mean, creating a new class just to annotate one variable in one function, just seems too much.

@@ -562,7 +573,7 @@ def _update_ctx(self, attrs):
for pair in col.rstrip(";").split(";"):
self.ctx[(i, j)].append(pair)

def _copy(self, deepcopy=False):
def _copy(self, deepcopy: bool = False):
Copy link
Member

Choose a reason for hiding this comment

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

elsewhere we use "deep" instead of "deepcopy". that wouldnt be something to change here, but might be worth looking into standardizing.

@gfyoung gfyoung added the Typing type annotations, mypy/pyright type checking label Dec 22, 2019
Copy link
Member

@WillAyd WillAyd left a comment

Choose a reason for hiding this comment

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

I think can add a few more looking at docstring

pandas/io/formats/style.py Outdated Show resolved Hide resolved
pandas/io/formats/style.py Outdated Show resolved Hide resolved
pandas/io/formats/style.py Outdated Show resolved Hide resolved
pandas/io/formats/style.py Outdated Show resolved Hide resolved
pandas/io/formats/style.py Outdated Show resolved Hide resolved
@ShaharNaveh
Copy link
Member Author

Thank you for the code suggestions @WillAyd

@simonjayhawkins simonjayhawkins added this to the 1.0 milestone Dec 23, 2019
Copy link
Member

@simonjayhawkins simonjayhawkins left a comment

Choose a reason for hiding this comment

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

Thanks @MomIsBestFriend lgtm. another pass (or two) with mypy --disallow-any-generics to add type parameters to the generics as suggested by @jbrockmendel and mypy --disallow-incomplete-defs to fill in some of the missing annotations would be great. @WillAyd

@@ -422,7 +433,7 @@ def format_attr(pair):
table_attributes=table_attr,
)

def format(self, formatter, subset=None, na_rep: Optional[str] = None):
def format(self, formatter, subset=None, na_rep: Optional[str] = None) -> "Styler":
Copy link
Member Author

@ShaharNaveh ShaharNaveh Dec 24, 2019

Choose a reason for hiding this comment

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

@simonjayhawkins Do you have any suggestion on how to annotate subset?

Or in other words how to represent IndexSlice in a typing form?

Copy link
Member

@simonjayhawkins simonjayhawkins Dec 31, 2019

Choose a reason for hiding this comment

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

from pandas.core.indexing import _IndexSlice (maybe in an if TYPE_CHECKING although not necessary)

Suggested change
def format(self, formatter, subset=None, na_rep: Optional[str] = None) -> "Styler":
def format(self, formatter, subset: Optional[_IndexSlice] = None, na_rep: Optional[str] = None) -> "Styler":

Copy link
Member Author

Choose a reason for hiding this comment

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

mypy is raising errors.

I stopped after a few, the error messages got pretty linear.

Any suggestions @simonjayhawkins ?


MYPY's logs:

For

subset: Optional[_IndexSlice]
$ mypy pandas
pandas/io/formats/style.py:486: error: Argument 1 to "len" has incompatible type "Optional[_IndexSlice]"; expected "Sized"
pandas/io/formats/style.py:487: error: Incompatible types in assignment (expression has type "Tuple[Optional[_IndexSlice], Any]", variable has type "Optional[_IndexSlice]")
Found 2 errors in 1 file (checked 844 source files)

For

subset: Tuple[Optional[_IndexSlice], Any]
$ mypy pandas
pandas/io/formats/style.py:436: error: Incompatible default for argument "subset" (default has type "None", argument has type "Tuple[Optional[_IndexSlice], Any]")
pandas/io/formats/style.py:487: error: Incompatible types in assignment (expression has type "Tuple[Tuple[Optional[_IndexSlice], Any], Any]", variable has type "Tuple[Optional[_IndexSlice], Any]")
Found 2 errors in 1 file (checked 844 source files)

For

subset: Tuple[Tuple[Optional[_IndexSlice], Any], Any]
$ mypy pandas
pandas/io/formats/style.py:436: error: Incompatible default for argument "subset" (default has type "None", argument has type "Tuple[Tuple[Optional[_IndexSlice], Any], Any]")
pandas/io/formats/style.py:487: error: Incompatible types in assignment (expression has type "Tuple[Tuple[Tuple[Optional[_IndexSlice], Any], Any], Any]", variable has type "Tuple[Tuple[Optional[_IndexSlice], Any], Any]")
Found 2 errors in 1 file (checked 844 source files)

For

subset: Tuple[Tuple[Tuple[Optional[_IndexSlice], Any], Any], Any]
$ mypy pandas
pandas/io/formats/style.py:436: error: Incompatible default for argument "subset" (default has type "None", argument has type "Tuple[Tuple[Tuple[Optional[_IndexSlice], Any], Any], Any]")
pandas/io/formats/style.py:487: error: Incompatible types in assignment (expression has type "Tuple[Tuple[Tuple[Tuple[Optional[_IndexSlice], Any], Any], Any], Any]", variable has type "Tuple[Tuple[Tuple[Optional[_IndexSlice], Any], Any], Any]")
Found 2 errors in 1 file (checked 844 source files)

pandas/io/formats/style.py Outdated Show resolved Hide resolved
pandas/io/formats/style.py Outdated Show resolved Hide resolved
return "{key}={value}".format(**pair)

# for sparsifying a MultiIndex
idx_lengths = _get_level_lengths(self.index)
col_lengths = _get_level_lengths(self.columns, hidden_columns)

cell_context = dict()
cell_context: Dict = dict()
Copy link
Member

Choose a reason for hiding this comment

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

subtypes

Copy link
Member Author

Choose a reason for hiding this comment

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

mypy doesn't complain for what ever I put in the subtypes.

Anyway, I don't see cell_context being inserted with any values but in few places .get() gets called on it, I am very confused.

Copy link
Member

Choose a reason for hiding this comment

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

Can you specify what the complaint is?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, I tried both with stubgen and MonkeyType and they didn't help. Also there is not really a good way to know if the types are actually good or not because what no matter what I put in it mypy won't complain.

Plus, this annotation really doesn't add that much, so I dropped it.

Unless you have a different opinion about dropping that annotation.

@@ -611,7 +624,9 @@ def _compute(self):
r = func(self)(*args, **kwargs)
return r

def _apply(self, func, axis=0, subset=None, **kwargs):
def _apply(
self, func: Callable, axis: Optional[Axis] = 0, subset=None, **kwargs
Copy link
Member

Choose a reason for hiding this comment

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

Can the args or return type of Callable be annotated?

Copy link
Member Author

Choose a reason for hiding this comment

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

Only the retrun type (from what I know)

Copy link
Member

Choose a reason for hiding this comment

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

Can you add return types then? Still very helpful

@@ -644,7 +659,9 @@ def _apply(self, func, axis=0, subset=None, **kwargs):
self._update_ctx(result)
return self

def apply(self, func, axis=0, subset=None, **kwargs):
def apply(
self, func: Callable, axis: Optional[Axis] = 0, subset=None, **kwargs
Copy link
Member

Choose a reason for hiding this comment

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

Same question on Callable

Copy link
Member Author

Choose a reason for hiding this comment

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

Only the retrun type (from what I know)

@jreback
Copy link
Contributor

jreback commented Dec 27, 2019

needs a rebase

@ShaharNaveh ShaharNaveh force-pushed the TYP-io-formats-style branch 4 times, most recently from 4668c1f to 0364330 Compare December 29, 2019 12:39
@ShaharNaveh
Copy link
Member Author

The errors were fixed.

Copy link
Member

@WillAyd WillAyd left a comment

Choose a reason for hiding this comment

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

Looks pretty good. We recently added Label to pandas._typing so should use that here in a few places (think I commented on all)

pandas/io/formats/excel.py Outdated Show resolved Hide resolved
pandas/io/formats/excel.py Outdated Show resolved Hide resolved
pandas/io/formats/excel.py Outdated Show resolved Hide resolved
pandas/io/formats/style.py Outdated Show resolved Hide resolved
pandas/io/formats/style.py Outdated Show resolved Hide resolved
pandas/io/formats/style.py Outdated Show resolved Hide resolved
return "{key}={value}".format(**pair)

# for sparsifying a MultiIndex
idx_lengths = _get_level_lengths(self.index)
col_lengths = _get_level_lengths(self.columns, hidden_columns)

cell_context = dict()
cell_context: Dict = dict()
Copy link
Member

Choose a reason for hiding this comment

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

Can you specify what the complaint is?

pandas/io/formats/style.py Show resolved Hide resolved
pandas/io/formats/style.py Show resolved Hide resolved
@ShaharNaveh
Copy link
Member Author

(think I commented on all)

You have :)

@@ -545,7 +555,7 @@ def render(self, **kwargs):
d.update(kwargs)
return self.template.render(**d)

def _update_ctx(self, attrs):
def _update_ctx(self, attrs: FrameOrSeries) -> None:
Copy link
Member

Choose a reason for hiding this comment

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

Does attrs here not have to be a DataFrame? Looks like it calls iterrows in the implementation which isn't defined for Series

Copy link
Member Author

@ShaharNaveh ShaharNaveh Jan 16, 2020

Choose a reason for hiding this comment

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

@WillAyd
Then the docstring is not correct.
I fixed both the type annotation and the docstring in 6a06e50

Copy link
Member

@WillAyd WillAyd left a comment

Choose a reason for hiding this comment

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

lgtm @MomIsBestFriend can you merge master one more time to be sure?

@simonjayhawkins over to you

@ShaharNaveh
Copy link
Member Author

Rebased


(unrelated but important)

@WillAyd have you saw what I wrote here?

@WillAyd WillAyd merged commit 059742b into pandas-dev:master Jan 20, 2020
@WillAyd
Copy link
Member

WillAyd commented Jan 20, 2020

Thanks @MomIsBestFriend

@ShaharNaveh ShaharNaveh deleted the TYP-io-formats-style branch January 20, 2020 20:03
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.

7 participants