-
-
Notifications
You must be signed in to change notification settings - Fork 18.1k
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: offsets.pyi #45331
TYP: offsets.pyi #45331
Conversation
Hello @twoertwein! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found: There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2022-02-17 03:27:46 UTC |
@@ -605,7 +605,8 @@ def asfreq(self, freq=None, how: str = "E") -> PeriodArray: | |||
|
|||
freq = Period._maybe_convert_freq(freq) | |||
|
|||
base1 = self.freq._period_dtype_code | |||
# error: "BaseOffset" has no attribute "_period_dtype_code" | |||
base1 = self.freq._period_dtype_code # type: ignore[attr-defined] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This error occurs a few times. BaseOffset does not have this attribute but almost all(?) sub-classes of BaseOffset have this attribute.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all(?)
Not all, no.
# treat this specifically as timedelta-NaT | ||
result = np.empty(self.shape, dtype=np.float64) | ||
result.fill(np.nan) | ||
return result | ||
|
||
# dispatch to Timedelta implementation | ||
result = other.__rfloordiv__(self._ndarray) | ||
return result | ||
return other.__rfloordiv__(self._ndarray) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Combined these two lines to not assign a new type to result
@overload | ||
def __add__(self, other: npt.NDArray[np.object_]) -> npt.NDArray[np.object_]: ... | ||
@overload | ||
def __add__(self: _BaseOffsetT, other: BaseOffset) -> _BaseOffsetT: ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These operators are not "perfect" but they might be good enough for now. Based on the current annotations, __add__(self: Year, other: Second)
is declared to return a Year
but it actually returns Second
@twoertwein can you merge main |
@jbrockmendel rebased and green |
@@ -219,7 +219,9 @@ def arithmetic_op(left: ArrayLike, right: Any, op): | |||
# (https://github.com/pandas-dev/pandas/issues/41165) | |||
_bool_arith_check(op, left, right) | |||
|
|||
res_values = _na_arithmetic_op(left, right, op) | |||
# error: Argument 1 to "_na_arithmetic_op" has incompatible type |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
likely avoidable if we inline should_extension_dispatch
freq._period_dtype_code == own_freq._period_dtype_code | ||
freq._period_dtype_code | ||
# error: "BaseOffset" has no attribute "_period_dtype_code" | ||
== own_freq._period_dtype_code # type: ignore[attr-defined] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think this is a perf optimization for dtype == self.dtype
that might no longer be necessary; i think we do this in PeriodDtype.eq now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should probably be done in a separate PR.
@twoertwein status here? |
I think it should be ready. The open two comments would require non-typing changes, which I would like to avoid in this PR. |
thanks @twoertwein |
hmm maybe should have rebase first, oh well, let's see |
This PR and #44922 are the last parts of #43744.