bpo-30516: Fix documentation issue with -timedelta in datetime #7348

farhaanbukhsh · 2018-06-03T07:47:12Z

This commit fixes the -timedelta overfllow issue not documented properly.

Signed-off-by: Farhaan Bukhsh farhaan.bukhsh@gmail.com

https://bugs.python.org/issue30516

vstinner · 2018-06-04T07:46:16Z

@abalkin @pganssle: Would you mind to review this datetime documentation change?

pganssle · 2018-06-04T15:52:32Z

Doc/library/datetime.rst

@@ -317,6 +317,11 @@ Notes:
  >>> print(_)
  -1 day, 19:00:00

+(6)
+   This isn't quite equivalent to timedelta + (-timedelta), because -timedelta in
+   isolation can overflow in cases where date1 - timedelta does not.


Should say "in cases where timedelta - timedelta does not"

pganssle · 2018-06-04T15:53:51Z

Doc/library/datetime.rst

+(6)
+   This isn't quite equivalent to timedelta + (-timedelta), because -timedelta in
+   isolation can overflow in cases where date1 - timedelta does not.
+   ``timedelta.seconds`` and ``timedelta.microseconds`` are ignored.


I think this part is only true for date2 = date1 - timedelta, and that part of the original footnote probably needs to remain in the "supported operations" section for datetime.date.

pganssle · 2018-06-04T15:58:20Z

I made a few notes. I'm somewhat ambivalent about including the footnote at all, since it's already covered by the notes on -t1 (footnotes 1 and 4), but I think it doesn't hurt anything to note the asymmetry between t1 - t2 and t1 + (-t2) (which is really just an asymmetry between t2 and -t2).

abalkin

+1 on @pganssle comments

bedevere-bot · 2018-06-04T17:13:34Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

farhaanbukhsh · 2018-06-05T02:57:33Z

@pganssle @abalkin I have updated the PR with the suggested comments, how does it look now?

pganssle · 2018-06-05T12:42:58Z

Doc/library/datetime.rst

-   This isn't quite equivalent to date1 + (-timedelta), because -timedelta in
-   isolation can overflow in cases where date1 - timedelta does not.
-   ``timedelta.seconds`` and ``timedelta.microseconds`` are ignored.
+   timedelta.seconds and timedelta.microseconds are ignored.


Just a note that has changed from:

`timedelta.seconds` and `timedelta.microseconds` are ignored

In the original they were in code blocks, but the documentation doesn't seem terribly consistent on this point. We should probably pick one and stick with it.

A cursory look at the rest of the documentation seems to indicate that "timedelta.X" is more common than "timedelta.X", but not by much, and often in expressions.

farhaanbukhsh · 2018-06-05T17:18:11Z

I have made the requested changes; please review again

bedevere-bot · 2018-06-05T17:18:13Z

Thanks for making the requested changes!

@abalkin: please review the changes made to this pull request.

pganssle · 2018-06-07T12:07:44Z

Doc/library/datetime.rst

-   This isn't quite equivalent to date1 + (-timedelta), because -timedelta in
-   isolation can overflow in cases where date1 - timedelta does not.
-   ``timedelta.seconds`` and ``timedelta.microseconds`` are ignored.
+   ``timedelta.seconds and timedelta.microseconds are ignored.``


Should be:

timedelta.seconds and timedelta.microseconds are ignored.

(Raw RST version):

``timedelta.seconds`` and ``timedelta.microseconds`` are ignored.

farhaanbukhsh · 2018-06-07T16:59:08Z

I have made the requested changes; please review again

bedevere-bot · 2018-06-07T16:59:10Z

Thanks for making the requested changes!

@abalkin: please review the changes made to this pull request.

vadmium · 2018-06-11T05:42:54Z

Doc/library/datetime.rst

@@ -317,6 +317,10 @@ Notes:
  >>> print(_)
  -1 day, 19:00:00

+(6)
+   This isn't quite equivalent to timedelta + (-timedelta), because -timedelta in
+   isolation can overflow in cases where timedelta - timedelta does not.


Which timedelta is which. Wouldn’t it make more sense to keep using t2 and t3?

I'm not sure it makes much difference which one is which:

The issue is that t1 - t2 == t1 + (-t2) for all cases except when t2 is timedelta.max, in which case the right side of the equation overflows before the subtraction because (-timedelta) is only valid on the interval (-timedelta.max, timedelta.max]. This is why timedelta + (-timedelta) and timedelta - timedelta are not equivalent.

On the other hand, if you interpret it as Python code timedelta + (-timedelta) would suggest the addition of the type objects themselves, and I don't think it would hurt to switch over to t1, t2 and t3 to make it clear that these are specific instances.

@abalkin Thoughts?

I agree that it best to follow the notation used in the body of the table in footnotes. How is this wording?

The expression t1 - t2 is not always the same as t1 + (-t2). In the case when t2 is timedelta.max, (-t2) in the second expression will overflow, while the first expression will produce an answer.

This actually makes things clearer, let me make this change and see how it looks.

abalkin · 2018-06-15T01:34:20Z

Doc/library/datetime.rst

@@ -317,6 +317,11 @@ Notes:
  >>> print(_)
  -1 day, 19:00:00

+(6)
+   The expression `t2 - t3` will always be equal to the expression `t2 + (-t3)` except
+   one condition when t3 is equal to `timedelta.max`, in that condition the former will produce result


Are you sure you want to repeat the word "condition" here? Why not simply "except when t3 is equal to timedelta.max, in that case ..."

Also, I feel you need an "a" before "result".

farhaanbukhsh · 2018-06-16T09:41:32Z

I have made the requested changes; please review again

bedevere-bot · 2018-06-16T09:41:34Z

Thanks for making the requested changes!

@abalkin: please review the changes made to this pull request.

farhaanbukhsh · 2018-06-21T15:37:22Z

I can't figure out why are tests failing here

pganssle · 2018-06-21T16:23:19Z

@farhaanbukhsh The test is failing because you're using single backticks instead of double backticks. In restructuredText, surrounding something with single backticks uses the "default role" (which is not :code: in my experience).

pganssle · 2018-06-21T16:25:25Z

Doc/library/datetime.rst

+   The expression `t2 - t3` will always be equal to the expression `t2 + (-t3)` except
+   one condition when t3 is equal to `timedelta.max`, in that case the former will produce a result
+   while the latter will overflow.
+


To fix the tests use:

(6) The expression ``t2 - t3`` will always be equal to the expression ``t2 + (-t3)`` except one condition when t3 is equal to ``timedelta.max``, in that case the former will produce a result while the latter will overflow.

I'll also note that "except one condition when" doesn't sound like idiomatic English to me. I would say "except the case where" or "except when t3 is equal to timedelta.max".

I also think the comma after timedelta.max should probably be a semicolon.

This commit fixes the -timedelta overfllow issue not documented properly. Signed-off-by: Farhaan Bukhsh <farhaan.bukhsh@gmail.com>

farhaanbukhsh · 2018-06-22T09:07:59Z

I have made the requested changes; please review again

bedevere-bot · 2018-06-22T09:08:01Z

Thanks for making the requested changes!

@abalkin: please review the changes made to this pull request.

pganssle

LGTM.

@farhaanbukhsh Thanks for dealing with all the pickiness! It's patient people like you who make CPython the high quality software that it is.

pganssle · 2018-06-22T13:30:48Z

I think this can be backported to 3.7, probably 3.6 as well.

farhaanbukhsh · 2018-06-22T14:01:14Z

@pganssle Thank it means a lot coming from you 😄, although I keep on looking for issues to solve.

miss-islington · 2018-07-04T18:42:58Z

Thanks @farhaanbukhsh for the PR, and @abalkin for merging it 🌮🎉.. I'm working now to backport this PR to: 3.6.
🐍🍒⛏🤖

miss-islington · 2018-07-04T18:42:58Z

Thanks @farhaanbukhsh for the PR, and @abalkin for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖

bedevere-bot · 2018-07-04T18:43:24Z

GH-8092 is a backport of this pull request to the 3.6 branch.

…nGH-7348) This commit fixes the -timedelta overfllow issue not documented properly. Signed-off-by: Farhaan Bukhsh <farhaan.bukhsh@gmail.com> (cherry picked from commit 5b6e49a) Co-authored-by: Farhaan Bukhsh <farhaan.bukhsh@gmail.com>

bedevere-bot · 2018-07-04T18:44:14Z

GH-8093 is a backport of this pull request to the 3.7 branch.

…nGH-7348) This commit fixes the -timedelta overfllow issue not documented properly. Signed-off-by: Farhaan Bukhsh <farhaan.bukhsh@gmail.com> (cherry picked from commit 5b6e49a) Co-authored-by: Farhaan Bukhsh <farhaan.bukhsh@gmail.com>

…) (GH-8092) This commit fixes the -timedelta overfllow issue not documented properly. Signed-off-by: Farhaan Bukhsh <farhaan.bukhsh@gmail.com> (cherry picked from commit 5b6e49a) Co-authored-by: Farhaan Bukhsh <farhaan.bukhsh@gmail.com>

…) (GH-8093) This commit fixes the -timedelta overfllow issue not documented properly. Signed-off-by: Farhaan Bukhsh <farhaan.bukhsh@gmail.com> (cherry picked from commit 5b6e49a) Co-authored-by: Farhaan Bukhsh <farhaan.bukhsh@gmail.com>

the-knights-who-say-ni added the CLA signed label Jun 3, 2018

bedevere-bot added the awaiting review label Jun 3, 2018

vstinner added the skip news label Jun 4, 2018

vstinner requested a review from abalkin June 4, 2018 07:44

vstinner requested a review from vadmium June 4, 2018 07:48

pganssle reviewed Jun 4, 2018

View reviewed changes

abalkin requested changes Jun 4, 2018

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting review labels Jun 4, 2018

farhaanbukhsh force-pushed the fix-datetime-documentation branch from eb26aaa to 7906aa6 Compare June 5, 2018 02:55

abalkin approved these changes Jun 5, 2018

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting changes labels Jun 5, 2018

pganssle reviewed Jun 5, 2018

View reviewed changes

farhaanbukhsh force-pushed the fix-datetime-documentation branch from 7906aa6 to 25b5980 Compare June 5, 2018 17:17

bedevere-bot added awaiting change review and removed awaiting merge labels Jun 5, 2018

pganssle reviewed Jun 7, 2018

View reviewed changes

farhaanbukhsh force-pushed the fix-datetime-documentation branch from 25b5980 to cc4b3d8 Compare June 7, 2018 16:55

vadmium reviewed Jun 11, 2018

View reviewed changes

farhaanbukhsh force-pushed the fix-datetime-documentation branch from cc4b3d8 to 875b530 Compare June 13, 2018 04:06

abalkin reviewed Jun 15, 2018

View reviewed changes

farhaanbukhsh force-pushed the fix-datetime-documentation branch from 875b530 to 4cf21e6 Compare June 16, 2018 09:39

pganssle reviewed Jun 21, 2018

View reviewed changes

bpo-30516: Fix documentation issue with -timedelta in datetime

dec4bff

This commit fixes the -timedelta overfllow issue not documented properly. Signed-off-by: Farhaan Bukhsh <farhaan.bukhsh@gmail.com>

farhaanbukhsh force-pushed the fix-datetime-documentation branch from 4cf21e6 to dec4bff Compare June 22, 2018 04:22

pganssle approved these changes Jun 22, 2018

View reviewed changes

abalkin merged commit 5b6e49a into python:master Jul 4, 2018

bedevere-bot removed the awaiting change review label Jul 4, 2018

abalkin self-assigned this Jul 4, 2018

abalkin added needs backport to 3.6 labels Jul 4, 2018

bedevere-bot removed the needs backport to 3.6 label Jul 4, 2018

bedevere-bot removed the needs backport to 3.7 label Jul 4, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-30516: Fix documentation issue with -timedelta in datetime #7348

bpo-30516: Fix documentation issue with -timedelta in datetime #7348

farhaanbukhsh commented Jun 3, 2018 •

edited by bedevere-bot

Loading

vstinner commented Jun 4, 2018

pganssle Jun 4, 2018

pganssle Jun 4, 2018 •

edited

Loading

pganssle commented Jun 4, 2018

abalkin left a comment

bedevere-bot commented Jun 4, 2018

farhaanbukhsh commented Jun 5, 2018

pganssle Jun 5, 2018

farhaanbukhsh commented Jun 5, 2018

bedevere-bot commented Jun 5, 2018

pganssle Jun 7, 2018 •

edited

Loading

farhaanbukhsh commented Jun 7, 2018

bedevere-bot commented Jun 7, 2018

vadmium Jun 11, 2018

pganssle Jun 12, 2018 •

edited

Loading

abalkin Jun 12, 2018

farhaanbukhsh Jun 13, 2018

abalkin Jun 15, 2018

farhaanbukhsh Jun 15, 2018

farhaanbukhsh commented Jun 16, 2018

bedevere-bot commented Jun 16, 2018

farhaanbukhsh commented Jun 21, 2018

pganssle commented Jun 21, 2018

pganssle Jun 21, 2018 •

edited

Loading

farhaanbukhsh commented Jun 22, 2018

bedevere-bot commented Jun 22, 2018

pganssle left a comment

pganssle commented Jun 22, 2018

farhaanbukhsh commented Jun 22, 2018

miss-islington commented Jul 4, 2018

miss-islington commented Jul 4, 2018

bedevere-bot commented Jul 4, 2018

bedevere-bot commented Jul 4, 2018

bpo-30516: Fix documentation issue with -timedelta in datetime #7348

bpo-30516: Fix documentation issue with -timedelta in datetime #7348

Conversation

farhaanbukhsh commented Jun 3, 2018 • edited by bedevere-bot Loading

vstinner commented Jun 4, 2018

pganssle Jun 4, 2018

Choose a reason for hiding this comment

pganssle Jun 4, 2018 • edited Loading

Choose a reason for hiding this comment

pganssle commented Jun 4, 2018

abalkin left a comment

Choose a reason for hiding this comment

bedevere-bot commented Jun 4, 2018

farhaanbukhsh commented Jun 5, 2018

pganssle Jun 5, 2018

Choose a reason for hiding this comment

farhaanbukhsh commented Jun 5, 2018

bedevere-bot commented Jun 5, 2018

pganssle Jun 7, 2018 • edited Loading

Choose a reason for hiding this comment

farhaanbukhsh commented Jun 7, 2018

bedevere-bot commented Jun 7, 2018

vadmium Jun 11, 2018

Choose a reason for hiding this comment

pganssle Jun 12, 2018 • edited Loading

Choose a reason for hiding this comment

abalkin Jun 12, 2018

Choose a reason for hiding this comment

farhaanbukhsh Jun 13, 2018

Choose a reason for hiding this comment

abalkin Jun 15, 2018

Choose a reason for hiding this comment

farhaanbukhsh Jun 15, 2018

Choose a reason for hiding this comment

farhaanbukhsh commented Jun 16, 2018

bedevere-bot commented Jun 16, 2018

farhaanbukhsh commented Jun 21, 2018

pganssle commented Jun 21, 2018

pganssle Jun 21, 2018 • edited Loading

Choose a reason for hiding this comment

farhaanbukhsh commented Jun 22, 2018

bedevere-bot commented Jun 22, 2018

pganssle left a comment

Choose a reason for hiding this comment

pganssle commented Jun 22, 2018

farhaanbukhsh commented Jun 22, 2018

miss-islington commented Jul 4, 2018

miss-islington commented Jul 4, 2018

bedevere-bot commented Jul 4, 2018

bedevere-bot commented Jul 4, 2018

farhaanbukhsh commented Jun 3, 2018 •

edited by bedevere-bot

Loading

pganssle Jun 4, 2018 •

edited

Loading

pganssle Jun 7, 2018 •

edited

Loading

pganssle Jun 12, 2018 •

edited

Loading

pganssle Jun 21, 2018 •

edited

Loading