Skip to content

gh-126883: Add check that timezone fields are in range for datetime.fromisoformat - #127242

Merged
pganssle merged 13 commits into
python:mainfrom
donbarbos:issue-126883
May 19, 2025
Merged

gh-126883: Add check that timezone fields are in range for datetime.fromisoformat#127242
pganssle merged 13 commits into
python:mainfrom
donbarbos:issue-126883

Conversation

@donbarbos

@donbarbosdonbarbos commented Nov 25, 2024•
edited by bedevere-app Bot
Loading

Copy link
Copy Markdown
Contributor
  1. Update C implementation
  2. Update Python implementation
  3. Add more use cases to tests

Also error output for the time.fromisoformat function was fixed as in datetime.fromisoformat in Python implementation

@bedevere-appbedevere-appBot mentioned this pull request Nov 25, 2024
Closed
@bedevere-app

bedevere-appBot commented Nov 25, 2024

Copy link
Copy Markdown

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

@pganssle

pganssle commented Nov 25, 2024

Copy link
Copy Markdown
Member

I haven't had a chance for a through review yet, but I'm in favor of the general approach and a casual glance looks good.

Can you add a news blurb (credit yourself appropriately unless you don't want that for some reason), and if you aren't already in ACKS please add yourself there (again unless you don't want to be acknowledged for some reason).

@donbarbos

donbarbos commented Nov 26, 2024

Copy link
Copy Markdown
ContributorAuthor

@pganssle done 👍
I think it's now ready for review and merging.
And I'll soon send a PR to solve the rest of the problems described here #127260 and I would be grateful if you answered in the issue.

@donbarbosdonbarbos mentioned this pull request Nov 30, 2024
Merged
@donbarbos

donbarbos commented Feb 3, 2025

Copy link
Copy Markdown
ContributorAuthor

@erlend-aasland maybe you have free time to review and merge it

erlend-aasland
erlend-aasland reviewed Feb 5, 2025
View reviewed changes

@erlend-aaslanderlend-aasland left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you please remove any unneeded style changes?

Comment threadLib/_pydatetime.py Outdated
donbarbosand others added 3 commits February 5, 2025 08:38
@donbarbos@erlend-aasland
Update Lib/_pydatetime.py
cc0d774
Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
@donbarbos
Revert style changes
9adfc6e
@donbarbos
Merge branch 'main' into issue-126883
eac790c
@donbarbos

donbarbos commented Apr 18, 2025

Copy link
Copy Markdown
ContributorAuthor

cc @pganssle@erlend-aasland
sorry for another ping but this was one of my first PRs and it's a matter of principle :)

pganssle
pganssle reviewed May 19, 2025
View reviewed changes

@pgansslepganssle left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Some minor nits, but overall looks good!

Comment threadLib/_pydatetime.py
Comment threadLib/_pydatetime.py Outdated
Comment threadModules/_datetimemodule.c Outdated
Comment threadLib/test/datetimetester.py
@donbarbos@pganssle
Apply suggestions from code review
5580d17
Co-authored-by: Paul Ganssle <1377457+pganssle@users.noreply.github.com>
@donbarbos
donbarbos requested a review from pganssleMay 19, 2025 16:12
@pganssle
pganssle merged commit 71c42b7 into python:mainMay 19, 2025
@pganssle

pganssle commented May 19, 2025

Copy link
Copy Markdown
Member

Thanks @donbarbos, great work on this and sorry for the long delay.

@donbarbos

donbarbos commented May 19, 2025

Copy link
Copy Markdown
ContributorAuthor

thank you everyone for review

Pranjal095 pushed a commit to Pranjal095/cpython that referenced this pull request Jul 12, 2025
@donbarbos@erlend-aasland
@pganssle@Pranjal095
pythongh-126883: Add check that timezone fields are in range for `dat…
ef39860
…etime.fromisoformat` (python#127242)
It was previously possible to specify things like `+00:90:00` which would be equivalent to `+01:30:00`, but is not a valid ISO8601 string.
---------
Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
Co-authored-by: Paul Ganssle <1377457+pganssle@users.noreply.github.com>
taegyunkim pushed a commit to taegyunkim/cpython that referenced this pull request Aug 4, 2025
@donbarbos@erlend-aasland
@pganssle@taegyunkim
pythongh-126883: Add check that timezone fields are in range for `dat…
d1a71a4
…etime.fromisoformat` (python#127242)
It was previously possible to specify things like `+00:90:00` which would be equivalent to `+01:30:00`, but is not a valid ISO8601 string.
---------
Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
Co-authored-by: Paul Ganssle <1377457+pganssle@users.noreply.github.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pgansslepgansslepganssle approved these changes
@abalkinabalkinAwaiting requested review from abalkin
@erlend-aaslanderlend-aaslandAwaiting requested review from erlend-aasland

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@donbarbos@pganssle@erlend-aasland