Skip to content

gh-93267: Fix wrong result Documented in Date Arithmetic datetime #93506

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

Closed
wants to merge 2 commits into from

Conversation

180909
Copy link
Contributor

@180909 180909 commented Jun 5, 2022

@180909 180909 requested review from pganssle and abalkin as code owners June 5, 2022 02:58
@bedevere-bot bedevere-bot added docs Documentation in the Doc dir awaiting review labels Jun 5, 2022
@AA-Turner
Copy link
Member

I don't believe swapping the order fixes anything, as "removed" is commutative (at least in British English) -- e.g. your first cousin once removed could be your father's cousin or your son's cousin.

We could update to "away from" as per the issue, although personally I feel that looses a little clarity of expression. Regardless, you should update line 1148 ("tetime2 is a duration of timedelta removed from datetime1, moving forward in") to the same verbiage that we decide on.

cc: @ericvsmith

A

@AA-Turner AA-Turner added needs backport to 3.10 only security fixes needs backport to 3.11 only security fixes labels Jun 5, 2022
@@ -589,8 +589,8 @@ Supported operations:
+-------------------------------+----------------------------------------------+
| Operation | Result |
+===============================+==============================================+
| ``date2 = date1 + timedelta`` | *date2* is ``timedelta.days`` days removed |
| | from *date1*. (1) |
| ``date2 = date1 + timedelta`` | *date1* is ``timedelta.days`` days removed |
Copy link
Contributor

@akulakov akulakov Jun 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The result should be what will be the value of date2 because date2 is what being assigned to. Suggestion:

Results in `timedelta.days` added to `date1`.

Even though it seems a bit repetitive to restate the operation on the left, it's already very intuitive and it would confirm to the reader that the + operator here does the obvious thing and not something weird.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about *date2* will be ``timedelta.days`` after *date1*. I think "after" indicates the direction correctly.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds good to me, using "after" here makes sense.

@bedevere-bot
Copy link

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.

@ericvsmith
Copy link
Member

@180909: Do you want to finish this PR, or should we close it?

@180909 180909 closed this Jul 28, 2022
@ericvsmith
Copy link
Member

I think we can leave this issue open in case someone else wants to pick it up. It's just the PR that has apparently been abandoned. This seems like a good first issue for someone. I'll mention it on core-mentorship.

@ericvsmith ericvsmith reopened this Jul 28, 2022
@ericvsmith
Copy link
Member

Oops, this is the PR. I'm constantly getting PRs and issues confused. Reclosing.

@ericvsmith ericvsmith closed this Jul 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting changes docs Documentation in the Doc dir needs backport to 3.10 only security fixes needs backport to 3.11 only security fixes skip news
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants