CLN: Add types in a handful of places #29178
Conversation
pandas/io/excel/_odfreader.py
Outdated
| @@ -60,7 +61,7 @@ def get_sheet_by_name(self, name: str): | |||
| if table.getAttribute("name") == name: | |||
| return table | |||
|
|
|||
| raise ValueError("sheet {name} not found".format(name)) | |||
| raise ValueError("sheet {name} not found".format(name=name)) | |||
There was a problem hiding this comment.
This is being address in #27677 with more detail (i.e. tests) so would rather revert here and let that PR take care of it, especially since it is their first contribution
pandas/core/nanops.py
Outdated
|
|
||
|
|
||
| def _maybe_get_mask( | ||
| values: np.ndarray, skipna: bool, mask: Optional[np.ndarray] | ||
| ) -> Optional[np.ndarray]: | ||
| """ This function will compute a mask iff it is necessary. Otherwise, | ||
| """ | ||
| Compute a mask iff necessary. |
There was a problem hiding this comment.
iff --> "if and only if"
| else: | ||
| result = result.astype("m8[ns]").view(dtype) | ||
|
|
||
| return result | ||
|
|
||
|
|
||
| def _na_for_min_count(values, axis): | ||
| """Return the missing value for `values` | ||
| def _na_for_min_count(values, axis: Optional[int]): |
There was a problem hiding this comment.
we're requiring the caller to pass something though
| @@ -692,30 +692,34 @@ def factorize(values, sort=False, order=None, na_sentinel=-1, size_hint=None): | |||
|
|
|||
|
|
|||
| def value_counts( | |||
| values, sort=True, ascending=False, normalize=False, bins=None, dropna=True | |||
| values, | |||
There was a problem hiding this comment.
Can type this as ndarray I think
There was a problem hiding this comment.
the name = getattr(values, "name", None) suggests that it could be a Series or Index
There was a problem hiding this comment.
Do you think AnyArrayLike from pandas._typing is applicable here?
There was a problem hiding this comment.
im hoping to refactor such that this function (as many as possible in this module, really) doesn't handle Series or Index so prefer not to make this official
| sort: bool = True, | ||
| ascending: bool = False, | ||
| normalize: bool = False, | ||
| bins=None, |
There was a problem hiding this comment.
| bins=None, | |
| bins: Optional[bool] = None, |
(unless docstring is wrong)
There was a problem hiding this comment.
docstring says int. should it be bool?
There was a problem hiding this comment.
Meant Optional[int] though? Can do in a follow up
| ascending: bool = False, | ||
| normalize: bool = False, | ||
| bins=None, | ||
| dropna: bool = True, | ||
| ): |
There was a problem hiding this comment.
You can annotate return as "Series". Not sure it will type check but still helpful I think
| @@ -1340,7 +1340,7 @@ def _intersection_non_unique(self, other: "IntervalIndex") -> "IntervalIndex": | |||
|
|
|||
| return self[mask] | |||
|
|
|||
| def _setop(op_name, sort=None): | |||
| def _setop(op_name: str, sort=None): | |||
There was a problem hiding this comment.
Was going to suggest annotating sort but looks like unused? Maybe delete altogether?
Can annotate the return here at least as Callable would be helpful too (better with subscripting if not too much effort). At first glance I assume setop meant that this set some value and wouldn't return anything, but looks not to be the case
| result_shape = values.shape[:axis] + values.shape[axis + 1 :] | ||
| result = np.empty(result_shape, dtype=values.dtype) | ||
| result.fill(fill_value) | ||
| return result | ||
|
|
||
|
|
||
| def nanany(values, axis=None, skipna=True, mask=None): | ||
| def nanany(values, axis=None, skipna: bool = True, mask=None): |
There was a problem hiding this comment.
Not sure if applicable here but you could use the Axis definition from pandas._typing, if this can accept both integers and "index"/"columns"
Annotated bool return value for this and below would be great too
pandas/io/excel/_odfreader.py
Outdated
| @@ -60,7 +61,7 @@ def get_sheet_by_name(self, name: str): | |||
| if table.getAttribute("name") == name: | |||
| return table | |||
|
|
|||
| raise ValueError("sheet {name} not found".format(name)) | |||
| raise ValueError("sheet {name} not found".format(name=name)) | |||
There was a problem hiding this comment.
This is being address in #27677 with more detail (i.e. tests) so would rather revert here and let that PR take care of it, especially since it is their first contribution
|
Great thanks @jbrockmendel |
No description provided.