10000 bpo-37642: Update max and min offset in datetime module by nsiregar · Pull Request #14878 · python/cpython · GitHub
[go: up one dir, main page]
Skip to content

bpo-37642: Update max and min offset in datetime module #14878

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Aug 9, 2019
Merged

bpo-37642: Update max and min offset in datetime module #14878

merged 1 commit into from
Aug 9, 2019

Conversation

nsiregar
Copy link
Contributor
@nsiregar nsiregar commented Jul 21, 2019 •
edited by bedevere-bot
Loading

Update max and min offset for datetime.timezone to closer 24:00

https://bugs.python.org/issue37642

joernheissler
joernheissler reviewed Jul 21, 2019
View reviewed changes
pganssle
pganssle requested changes Jul 21, 2019
View reviewed changes
@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.

@nsiregar
Copy link
Contributor Author

I have made the requested changes; please review again

@bedevere-bot
Copy link

Thanks for making the requested changes!

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

@nsiregar
Copy link
Contributor Author

Looks like there is different implementation in max and min offset in _datetimemodule.c.

min offset in C is

cpython/Modules/_datetimemodule.c

Line 6487 in 35b87e6

delta = new_delta(-1, 60, 0, 1); /* -23:59 */

max offset in C is

cpython/Modules/_datetimemodule.c

Line 6496 in 35b87e6

delta = new_delta(0, (23 * 60 + 59) * 60, 0, 0); /* +23:59 */

Should I also update these?

@joernheissler
Copy link
Contributor

Looks like there is different implementation in max and min offset in _datetimemodule.c.
…
Should I also update these?

I think you should.

A shorter way to write the new limits would be timedelta(days=1, microseconds=-1) but I'm not sure about readability.

@nsiregar
Copy link
Contributor Author

i was plan to use new_delta(-1, 0, 1,1) for negative offset, is it okay?

@pganssle
Copy link
Member

Looks like there is different implementation in max and min offset in _datetimemodule.c.

I commented recently on the bpo issue, but I think we should keep timezone.min and timezone.max the same for this PR, which would allow us to backport it to Python 3.7 and 3.8. We should probably have a separate discussion about the scope and convenience of timezone.min and timezone.max, because I suspect that changing those to a value microsecond offsets could cause a lot of needless churn.

@nsiregar nsiregar force-pushed the bpo-37642-timezone-offset branch 2 times, most recently from df9064b to 717cccf Compare July 23, 2019 12:29
@nsiregar
Copy link
Contributor Author

I have made the requested changes; please review again

@bedevere-bot
Copy link

Thanks for making the requested changes!

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

pganssle
pganssle requested changes Jul 23, 2019
View reviewed changes
Copy link
Member
@pganssle pganssle left a comment

Choose a reason for hiding this comment

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

Ugh, I wrote this review yesterday and didn't realize I never submitted it.

Per this comment, I think we need to keep timezone.min and timezone.max the same for now. I would say to move the changes for timezone.min and timezone.max into another branch, but I think we need a new BPO issue for it because I think that needs more discussion.

@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.

pganssle
pganssle requested changes Jul 23, 2019
View reviewed changes
Lib/test/datetimetester.py Outdated
@@ -279,7 +280,7 @@ def test_constructor(self):
tz = timezone(subminute)
self.assertNotEqual(tz.utcoffset(None) % timedelta(minutes=1), 0)
# invalid offsets
for invalid in [timedelta(1, 1), timedelta(1)]:
for invalid in [timedelta(1, 1), timedelta(days=1, microseconds=1)]:
Copy link
Member

Choose a reason for hiding this comment

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

Why does this need to be changed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, looks like i forgot to restore it

@nsiregar nsiregar force-pushed the bpo-37642-timezone-offset branch from 717cccf to e7ca520 Compare July 23, 2019 15:06
@nsiregar
Copy link
Contributor Author

I have made the requested changes; please review again

@pganssle pganssle force-pushed the bpo-37642-timezone-offset branch from a29411b to cabad73 Compare August 9, 2019 13:54
@nsiregar @pganssle
bpo-37642: Update acceptable offsets in timezone
< 8000 input type="hidden" name="dropdown_direction" value="s" autocomplete="off" data-targets="batch-deferred-content.inputs" />
104ebe0 
This fixes an inconsistency between the Python and C implementations of
the datetime module. The pure python version of the code was not
accepting offsets greater than 23:59 but less than 24:00. This is an
accidental legacy of the original implementation, which was put in place
before tzinfo allowed sub-minute time zone offsets.

pythonGH-14878
@pganssle pganssle force-pushed the bpo-37642-timezone-offset branch from cabad73 to 104ebe0 Compare August 9, 2019 13:55
@pganssle pganssle merged commit 92c7e30 into python:master Aug 9, 2019
@miss-islington
Copy link
Contributor

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

@miss-islington
Copy link
Contributor

Sorry, @nsiregar and @pganssle, I could not cleanly backport this to 3.8 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 92c7e30adf5c81a54d6e5e555a6bdfaa60157a0d 3.8

@miss-islington
Copy link
Contributor

Sorry @nsiregar and @pganssle, I had trouble checking out the 3.7 backport branch.
Please backport using cherry_picker on command line.
cherry_picker 92c7e30adf5c81a54d6e5e555a6bdfaa60157a0d 3.7

@pganssle pganssle mentioned this pull request Aug 12, 2019
Merged
@bedevere-bot
Copy link

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

pganssle pushed a commit to pganssle/cpython that referenced this pull request Aug 12, 2019
@nsiregar @pganssle
bpo-37642: Update acceptable offsets in timezone (pythonGH-14878)
6154aed 
This fixes an inconsistency between the Python and C implementations of
the datetime module. The pure python version of the code was not
accepting offsets greater than 23:59 but less than 24:00. This is an
accidental legacy of the original implementation, which was put in place
before tzinfo allowed sub-minute time zone offsets.

pythonGH-14878

(cherry picked from commit 92c7e30)
pganssle pushed a commit to pganssle/cpython that referenced this pull request Aug 12, 2019
@nsiregar @pganssle
bpo-37642: Update acceptable offsets in timezone (pythonGH-14878)
d1116d3 
This fixes an inconsistency between the Python and C implementations of
the datetime module. The pure python version of the code was not
accepting offsets greater than 23:59 but less than 24:00. This is an
accidental legacy of the original implementation, which was put in place
before tzinfo allowed sub-minute time zone offsets.

pythonGH-14878

(cherry picked from commit 92c7e30)
@pganssle pganssle mentioned this pull request Aug 12, 2019
Merged
@bedevere-bot
Copy link

GH-15227 is a backport of this pull request to the 3.8 branch.

pganssle added a commit that referenced this pull request Aug 15, 2019
@pganssle
bpo-37642: Update acceptable offsets in timezone (GH-14878) (#15227)
27b38b9 
This fixes an inconsistency between the Python and C implementations of
the datetime module. The pure python version of the code was not
accepting offsets greater than 23:59 but less than 24:00. This is an
accidental legacy of the original implementation, which was put in place
before tzinfo allowed sub-minute time zone offsets.

GH-14878

(cherry picked from commit 92c7e30)
pganssle added a commit that referenced this pull request Aug 15, 2019
@pganssle
bpo-37642: Update acceptable offsets in timezone (GH-14878) (#15226)
ed44b84 
This fixes an inconsistency between the Python and C implementations of
the datetime module. The pure python version of the code was not
accepting offsets greater than 23:59 but less than 24:00. This is an
accidental legacy of the original implementation, which was put in place
before tzinfo allowed sub-minute time zone offsets.

GH-14878

(cherry picked from commit 92c7e30)
lisroach pushed a commit to lisroach/cpython that referenced this pull request Sep 10, 2019
@nsiregar @lisroach
bpo-37642: Update acceptable offsets in timezone (pythonGH-14878)
8b1f5b4 
This fixes an inconsistency between the Python and C implementations of
the datetime module. The pure python version of the code was not
accepting offsets greater than 23:59 but less than 24:00. This is an
accidental legacy of the original implementation, which was put in place
before tzinfo allowed sub-minute time zone offsets.

pythonGH-14878
merwok
merwok reviewed Dec 19, 2019
View reviewed changes EC13
Misc/ACKS
@@ -1881,3 +1881,4 @@ Aleksandr Balezin
Robert Leenders
Tim Hopper
Dan Lidral-Porter
Ngalim Siregar
Copy link
Member

Choose a reason for hiding this comment

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

Please note that we don’t append to this file, but keep it sorted alphabetically by author last name.

DinoV pushed a commit to DinoV/cpython that referenced this pull request Jan 14, 2020
@nsiregar @DinoV
bpo-37642: Update acceptable offsets in timezone (pythonGH-14878)
fbd5d22 
This fixes an inconsistency between the Python and C implementations of
the datetime module. The pure python version of the code was not
accepting offsets greater than 23:59 but less than 24:00. This is an
accidental legacy of the original implementation, which was put in place
before tzinfo allowed sub-minute time zone offsets.

pythonGH-14878
websurfer5 pushed a commit to websurfer5/cpython that referenced this pull request Jul 20, 2020
@nsiregar @websurfer5
bpo-37642: Update acceptable offsets in timezone (pythonGH-14878)
537566a 
This fixes an inconsistency between the Python and C implementations of
the datetime module. The pure python version of the code was not
accepting offsets greater than 23:59 but less than 24:00. This is an
accidental legacy of the original implementation, which was put in place
before tzinfo allowed sub-minute time zone offsets.

pythonGH-14878
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@merwok merwok merwok left review comments

@joernheissler joernheissler joernheissler left review comments

@pganssle pganssle pganssle approved these changes

@abalkin abalkin Awaiting requested review from abalkin

Assignees

@pganssle pganssle

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@nsiregar @bedevere-bot @joernheissler @pganssle @miss-islington @merwok @the-knights-who-say-ni
0