CLN: Removed overridden Timedelta properties (#18242) #18374
Conversation
| @@ -665,36 +665,6 @@ cdef class _Timedelta(timedelta): | |||
| else: | |||
| return "D" | |||
|
|
|||
| @property | |||
| def days(self): | |||
There was a problem hiding this comment.
technically then won't self._sign be wrong (not that we are actually using it here)? I guess this is ok
There was a problem hiding this comment.
The only place where self._sign gets used is in _repr_base, which calls _ensure_components at the start of the method to set self._sign. That happens irrespective of the property definitions.
If _repr_base were ever re-factored in the future it could definitely be missed. OK to open a separate issue to see if we can refactor the _repr_base call and make it more "future-proof" by removing the need for self._sign altogether?
FWIW I checked the test cases and didn't see one that explicitly handles the repr of negative Timedeltas, so I'll add one and re-push
There was a problem hiding this comment.
Scratch my comment on not having test cases - you'll see some in test_arithmetic that ensure we have the appropriate Timedelta repr, so I'm not planning to re-push unless you have additional changes you'd like.
Codecov Report
@@ Coverage Diff @@
## master #18374 +/- ##
=======================================
Coverage 91.38% 91.38%
=======================================
Files 164 164
Lines 49797 49797
=======================================
Hits 45508 45508
Misses 4289 4289
Continue to review full report at Codecov.
|
|
thanks! |
git diff upstream/master -u -- "*.py" | flake8 --diff