TYP: some (mainly primative) types for core.generic #29991
Conversation
| """True if the key is in the info axis""" | ||
| return key in self._info_axis | ||
|
|
||
| @property | ||
| def empty(self): | ||
| def empty(self) -> bool_t: |
There was a problem hiding this comment.
should we be generally standarizing on using bool_t rather than bool?
There was a problem hiding this comment.
IMO probably not necessary. probably only a few modules where this pattern is needed.
| @@ -1395,7 +1396,7 @@ def _set_axis_name(self, name, axis=0, inplace=False): | |||
| # ---------------------------------------------------------------------- | |||
| # Comparison Methods | |||
|
|
|||
| def _indexed_same(self, other): | |||
| def _indexed_same(self, other) -> bool: | |||
There was a problem hiding this comment.
why do we have bool here and above but bool_t below? i expected to need bool_t throughout
also i think other should have same type as self here
There was a problem hiding this comment.
Hmm yea curious what the pattern here is; do only things after def bool need the alias? Or do you know if this is no longer an issue with Py36 onwards?
There was a problem hiding this comment.
That's what appears to be the case. def bool is in the class namespace and doesn't effect the use of bool inside other methods see L8658. However, the functions signatures must be evaluated in the class namespace since any types added to the function signatures after the def bool need to use the alias.
There was a problem hiding this comment.
also i think
othershould have same type as self here
hmm, the types should be representative of the runtime behaviour. should pd.Series()._indexed_same(pd.DataFrame()) raise?
>>> pd.__version__
'0.26.0.dev0+1155.ged20822a5'
>>>
>>> pd.DataFrame()._indexed_same(pd.DataFrame())
True
>>>
>>> pd.Series()._indexed_same(pd.DataFrame())
True
>>>
>>> pd.Series()._indexed_same(pd.Series())
True
>>>
>>> pd.DataFrame()._indexed_same(pd.Series())
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "C:\Users\simon\pandas\pandas\core\generic.py", line 1400, in _indexed_same
self._get_axis(a).equals(other._get_axis(a)) for a in self._AXIS_ORDERS
File "C:\Users\simon\pandas\pandas\core\generic.py", line 1400, in <genexpr>
self._get_axis(a).equals(other._get_axis(a)) for a in self._AXIS_ORDERS
File "C:\Users\simon\pandas\pandas\core\generic.py", line 444, in _get_axis
name = self._get_axis_name(axis)
File "C:\Users\simon\pandas\pandas\core\generic.py", line 441, in _get_axis_name
raise ValueError("No axis named {0} for object type {1}".format(axis, cls))
ValueError: No axis named columns for object type <class 'pandas.core.series.Series'>
>>>
There was a problem hiding this comment.
I'd ask for bool_t to be made consistent across the module, totally OK with leaving other here as out of scope
|
Question about bool vs bool_t consistency, not a blocker, otherwise LGTM |
|
thanks @simonjayhawkins happy to address the bool/bool_t issue later; we should likely just standardize on bool_t no? |
mypy 0.750 has an experimental feature for static inference of annotations see https://mypy.readthedocs.io/en/latest/mypy_daemon.html#static-inference-of-annotations
have tried it out on functions in core.generic. The annotations add here are mainly the ones that matched the docstrings.