TYPING: check-untyped-defs for io.formats #28129
Conversation
pandas/io/formats/style.py
Outdated
|
|
||
| jinja2 = import_optional_dependency("jinja2", extra="DataFrame.style requires jinja2.") | ||
| ApplyArgs = Tuple[Callable[[FrameOrSeries], FrameOrSeries], Axis, Optional[_IndexSlice]] | ||
| Kwargs = Dict[str, Any] |
There was a problem hiding this comment.
Hmm not sure about this; I think would be good to limit the amount of global aliases in use.
Do you see a general use for this outside of the ninja example below?
There was a problem hiding this comment.
no, but alias used for readability of L130
if we don't use aliases, what's your preference for formatting?
There was a problem hiding this comment.
i've retained the aliases for readability since "Type aliases are useful for simplifying complex type signatures" https://docs.python.org/3/library/typing.html#type-aliases
but moved them to just before the class where they are used. This pattern is used in https://github.com/python/typeshed/blob/master/stdlib/2and3/codecs.pyi
pandas/io/formats/style.py
Outdated
| _ApplyArgs = Tuple[ | ||
| Callable[[FrameOrSeries], FrameOrSeries], Axis, Optional[_IndexSlice] | ||
| ] | ||
| _Kwargs = Dict[str, Any] |
There was a problem hiding this comment.
I don't think we should create this for Args and Kwargs. Doesn't PEP 484 have a different way of handling anyway?
https://www.python.org/dev/peps/pep-0484/#arbitrary-argument-lists-and-default-argument-values
There was a problem hiding this comment.
The Kwargs alias is not for annotating kwargs in function signiture. The kwargs dict is stored in a local variable. I appreciate this could be confusing.
It was to make self._todo = [] # type: List[Tuple[Callable, _ApplyArgs, _Kwargs]] more readable.
i'll inline _Kwargs to avoid this confusion.
pandas/io/formats/style.py
Outdated
| @@ -1332,7 +1348,8 @@ def from_custom_template(cls, searchpath, name): | |||
| """ | |||
| loader = jinja2.ChoiceLoader([jinja2.FileSystemLoader(searchpath), cls.loader]) | |||
|
|
|||
| class MyStyler(cls): | |||
| # https://github.com/python/mypy/issues/2477 | |||
| class MyStyler(cls): # type: ignore | |||
There was a problem hiding this comment.
Can't this just be a Type of the self class?
There was a problem hiding this comment.
yes. with a cast. i'll leave the comment to the mypy issue.
There was a problem hiding this comment.
even with the correct revealed type for cls, mypy still not happy with the dynamic class creation.
pandas\io\formats\style.py:1351: error: Revealed type is 'Type[pandas.io.formats.style.Styler]'
pandas\io\formats\style.py:1353: error: Invalid type "cls"
pandas\io\formats\style.py:1353: error: Invalid base class
There was a problem hiding this comment.
to keep things moving, will revert the addition of type:ignore here for now.
pandas/io/formats/style.py
Outdated
| axis: Axis = 0, | ||
| subset: Optional[_IndexSlice] = None, | ||
| **kwargs | ||
| ): |
There was a problem hiding this comment.
Can you annotate the return ("Styler"?)
There was a problem hiding this comment.
OK, but may need to change to generic self https://mypy.readthedocs.io/en/latest/generics.html#generic-methods-and-generic-self because of MyStyler subclass when adding type annotations to functions in this module in future pass. not sure until look at this.
The only reason I added type annotations to the arguments of apply was to ensure they were compatible with _ApplyArgs otherwise this pass would have been to fix check-untyped-def errors only
| @@ -47,6 +49,11 @@ def _mpl(func): | |||
| raise ImportError(no_mpl_message.format(func.__name__)) | |||
|
|
|||
|
|
|||
| _ApplyArgs = Tuple[ | |||
There was a problem hiding this comment.
I assume this is a function of the code itself and not necessarily the annotation, but I find this pretty tough to digest. Is it documented already somewhere how this works? If not can you comment?
There was a problem hiding this comment.
I'd rather not in this pass. I don't know this module well enough yet.
This pass is just check-untyped-defs as "Working through this flag in advance may lead to less surprises as annotations are added" xref #27568
Also the sooner we resolve these issues, the sooner we can enable this setting in CI to ensure that code modified in functions without annotations is checked for PRs.
xref #27568