TYP: disallow comment-based annotation syntax #29741
Conversation
simonjayhawkins
added
Code Style
pandas/util/_decorators.py
Outdated
| @@ -328,8 +328,9 @@ def my_dog(has='fleas'): | |||
| """ | |||
|
|
|||
| def __init__(self, addendum: Optional[str], join: str = "", indents: int = 0): | |||
| self.addendum: Optional[str] | |||
There was a problem hiding this comment.
For instance variables I think should follow PEP 526 (@jbrockmendel had this on another PR) so this would move to the class space without definition:
https://www.python.org/dev/peps/pep-0526/#class-and-instance-variable-annotations
There was a problem hiding this comment.
sure. is that the intention of the PEP? or for compatibility with stubs and dataclasses?
the first line reads "Type annotations can also be used to annotate class and instance variables in class bodies and methods."
and since we have methods, why not annotate where they are defined.
in __init__ it makes no difference but in say DataFrameFormatter, self.max_rows_adj is only added in _chk_truncate and so would be wrong to suggest that the variable always exists and that a hasattr check is not necessary?
There was a problem hiding this comment.
The PEP describes all variable annotations. Admittedly haven't gone over every detail so if you see something counter lmk
There was a problem hiding this comment.
and so would be wrong to suggest that the variable always exists and that a hasattr check is not necessary?
I think this is part of the benefit of having the annotations; the variable should be set to None in __init__ in this kind of case. In e.g. io.pytables im finding class-level annotations really helpful in classes that set their attributes in a bunch of places.
There was a problem hiding this comment.
sure. refactoring the code may be beneficial. my comment was regarding adding annotations to the existing code without modification.
should we refactor in parallel to adding type hints, or refactor in a separate PR?
|
@WillAyd happy to have specific questions in a followup for @simonjayhawkins can you rebase. |
|
@simonjayhawkins one more rebase and merge on green. |
|
sure |
No description provided.