BUG: GH24011 - Rich comparisons of Timestamps now return NotImplemented #24021
Conversation
|
Hello @AlexandreDecan! 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 2019-04-01 12:13:42 UTC |
|
Tests did not fail locally. I will have a closer look to see why. |
|
@AlexandreDecan did this become more time-sensitive, or can it still wait until January? |
It can wait until January, I have temporary workarounds. For simplicity, should I close this PR until January? |
No, it just means I won't feel bad about taking my time to form an opinion. |
|
You should never feel bad about taking time to form an opinion 😉 |
Codecov Report
@@ Coverage Diff @@
## master #24021 +/- ##
===========================================
- Coverage 92.31% 42.45% -49.87%
===========================================
Files 161 161
Lines 51549 51561 +12
===========================================
- Hits 47586 21888 -25698
- Misses 3963 29673 +25710
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #24021 +/- ##
==========================================
- Coverage 91.82% 91.81% -0.01%
==========================================
Files 175 175
Lines 52580 52581 +1
==========================================
- Hits 48279 48276 -3
- Misses 4301 4305 +4
Continue to review full report at Codecov.
|
|
@jbrockmendel so deferring this to 0.25, can you PR for reverting the Timedelta change. |
|
Closing as stale. Ping if you'd like to continue |
|
Ping!? Waiting for info in #24011 |
|
@AlexandreDecan sounds good I've reopened. FWIW this would go in on 0.25 at the earliest at which point we drop Python2 support, so any compat issues there are not really a concern. Briefly read through the rest of the issue and wasn't clear what info you might be waiting for so let me know if I overlooked. Otherwise feel free to merge master |
|
It wasn't clear to me what is the plan for this issue/PR. I'll add some tests to this PR asap (probably not before tomorrow). |
|
I forgot that there are already some tests. I'll merge master, and update the what's new entry according to the guidelines (need to find them first ;-) |
|
What about the existing tests that are explicitly failing because of this change? Could I remove/adapt them, or should this be part of another PR? |
|
I've just fixed the test. Sorry for that, I should have checked before pushing the commits ;-) |
|
Try it out. It may work.
…On Tue, Mar 5, 2019 at 5:57 AM Alexandre Decan ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In doc/source/whatsnew/v0.25.0.rst
<#24021 (comment)>:
> @@ -33,6 +33,9 @@ Backwards incompatible API changes
.. _whatsnew_0250.api_breaking.utc_offset_indexing:
+- Comparing :class:`~Timestamp.Timestamp` with unsupported objects now returns ``NotImplemented`` instead of raising ``TypeError``. This implies that unsupported rich comparisons are delegated to the other object, and is now consistent with Python 3 behavior for ``datetime`` objects (:issue:`24011`)
I think NotImplemented is an object, not a class (see
https://github.com/python/cpython/blob/4d61e6e3b802399be62a521d6fa785698cb670b5/Objects/object.c#L1730).
Am I wrong?
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#24021 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABQHIksDQENa8fhBKNWCg9MeqGGdGuPLks5vTluxgaJpZM4Y8HOu>
.
|
|
If not, you can include a ref to
https://docs.python.org/3/library/constants.html#NotImplemented
On Tue, Mar 5, 2019 at 6:01 AM Tom Augspurger <[email protected]>
wrote:
… Try it out. It may work.
On Tue, Mar 5, 2019 at 5:57 AM Alexandre Decan ***@***.***>
wrote:
> ***@***.**** commented on this pull request.
> ------------------------------
>
> In doc/source/whatsnew/v0.25.0.rst
> <#24021 (comment)>:
>
> > @@ -33,6 +33,9 @@ Backwards incompatible API changes
>
> .. _whatsnew_0250.api_breaking.utc_offset_indexing:
>
> +- Comparing :class:`~Timestamp.Timestamp` with unsupported objects now returns ``NotImplemented`` instead of raising ``TypeError``. This implies that unsupported rich comparisons are delegated to the other object, and is now consistent with Python 3 behavior for ``datetime`` objects (:issue:`24011`)
>
> I think NotImplemented is an object, not a class (see
> https://github.com/python/cpython/blob/4d61e6e3b802399be62a521d6fa785698cb670b5/Objects/object.c#L1730).
> Am I wrong?
>
> —
> You are receiving this because you commented.
> Reply to this email directly, view it on GitHub
> <#24021 (comment)>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/ABQHIksDQENa8fhBKNWCg9MeqGGdGuPLks5vTluxgaJpZM4Y8HOu>
> .
>
|
|
I tested and it works. Thanks! |
|
can you merge master |
|
pls merge master |
|
Done. How many times will I have to merge master? I already did it 12 days ago, when you asked... :) |
|
pr was failing the CI
we have quite a number of PRs and they are need updating especially when thing change. 12 days is a long time. |
|
@jbrockmendel if you have comments |
|
thanks @AlexandreDecan |
git diff upstream/master -u -- "*.py" | flake8 --diff