◐ Shell
reader mode source ↗
Skip to content

gh-109798: Normalize _datetime and datetime error messages#127345

Merged
pganssle merged 31 commits into
python:mainfrom
donbarbos:issue-109798
Feb 12, 2025
Merged

gh-109798: Normalize _datetime and datetime error messages#127345
pganssle merged 31 commits into
python:mainfrom
donbarbos:issue-109798

Conversation

@donbarbos

@donbarbos donbarbos commented Nov 27, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

I only updated the messages, I fixed everything I found.
From these errors:

ValueError: ('year must be in 1..9999', {year})
ValueError: Year is out of range: {year}
ValueError: year {year} is out of range
ValueError: Year {year} is out of range

i made this one error:

ValueError: year must be in 1..9999, not {year}

and I made the same message template for the fields year, month, day, hour, minute, second, microsecond and fold because I decided that it was worth leaving the field that was received in the output and at the same time using one string instead of tuple.
with the rest of the error messages it's easier, I just reduced them to one type

@bedevere-app

bedevere-app Bot commented Nov 27, 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.

@donbarbos donbarbos changed the title Update error messages to be the same in datetime Nov 27, 2024
@bedevere-app bedevere-app Bot mentioned this pull request Nov 27, 2024
Closed
@erlend-aasland

erlend-aasland commented Nov 29, 2024

Copy link
Copy Markdown
Contributor

BTW, please do not force-push; it makes reviewing harder. Moreover, all commits are squashed upon merge anyway, so there's no need for the PR/branch to be cluttered with amendment commits. See also the devguide.

@donbarbos

donbarbos commented Nov 29, 2024

Copy link
Copy Markdown
Contributor Author

@erlend-aasland sorry, i got it.
These are all the inconsistencies in error messages I have found so far.
I'm done with this and that's enough for now.

@erlend-aasland erlend-aasland changed the title gh-109798: Update error messages to be the same in datetime Nov 29, 2024
donbarbos and others added 6 commits November 29, 2024 13:30
@donbarbos @erlend-aasland
Update Modules/_datetimemodule.c …
f691251 
Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
@donbarbos @erlend-aasland
Update Modules/_datetimemodule.c …
d8973cf 
Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
@donbarbos @erlend-aasland
Update Modules/_datetimemodule.c …
5eea62f 
Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
@donbarbos
Update Modules/_datetimemodule.c for optimisation
79543cc
@donbarbos
Revert last update Modules/_datetimemodule.c
d174497
@donbarbos
Update Modules/_datetimemodule.c
498c4ba
ZeroIntensity
ZeroIntensity reviewed Nov 30, 2024
View reviewed changes

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

Hide comment

Some nitpicks, and two notes:

  • I'm happy with doing this, but we do need to be aware that this is a slight breaking change for anybody relying on the exception messages. They shouldn't be doing that, but we need to be aware of it anyway.
  • It's fine right now, but PyErr_Format with %R can have unintended side effects if the __repr__ of the passed object is evil. Again, not a problem here as far as I can tell, but it's definitely worth noting.

24 hidden items Load more…
donbarbos and others added 6 commits November 30, 2024 04:17
@donbarbos @ZeroIntensity
Update Lib/_pydatetime.py …
4d31d33 
Co-authored-by: Peter Bierma <zintensitydev@gmail.com>
@donbarbos @ZeroIntensity
Update Lib/_pydatetime.py …
f05ebba 
Co-authored-by: Peter Bierma <zintensitydev@gmail.com>
@donbarbos @ZeroIntensity
Update Lib/_pydatetime.py …
f840105 
Co-authored-by: Peter Bierma <zintensitydev@gmail.com>
@donbarbos @ZeroIntensity
Update Lib/_pydatetime.py …
61c95a5 
Co-authored-by: Peter Bierma <zintensitydev@gmail.com>
@donbarbos @ZeroIntensity
Update Lib/_pydatetime.py …
2ab77b3 
Co-authored-by: Peter Bierma <zintensitydev@gmail.com>
@donbarbos
Update Lib/_pydatetime.py
b1e272a
ZeroIntensity
ZeroIntensity reviewed Nov 30, 2024
View reviewed changes

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

Hide comment

Now, how far fetched is it to ask for some assertRaisesRegex tests 😄

@donbarbos

donbarbos commented Nov 30, 2024

Copy link
Copy Markdown
Contributor Author

@ZeroIntensity i added tests ✅ ,
but if you need tests for the fromisoformat method - need to merge this PR #127242 because it solves many problems, for example, the rules for different implementations may seem different

ZeroIntensity
ZeroIntensity reviewed Nov 30, 2024
View reviewed changes

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

Hide comment

Getting close to ready :)

donbarbos and others added 4 commits December 1, 2024 07:43
@donbarbos @ZeroIntensity
Update _pydatetime.py …
2827514 
Co-authored-by: Peter Bierma <zintensitydev@gmail.com>
@donbarbos @ZeroIntensity
Update _pydatetime.py …
cd3bdc1 
Co-authored-by: Peter Bierma <zintensitydev@gmail.com>
@donbarbos
Change but got to not
9915dfe
@donbarbos
Correct line break
1da5a3a
@donbarbos

donbarbos commented Dec 20, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@erlend-aasland could you merge it? (if you have time :-)

@ZeroIntensity

ZeroIntensity commented Dec 20, 2024

Copy link
Copy Markdown
Member

FYI, most of the core devs are done for the holidays. You'll have to wait until after the new year probably.

pganssle
pganssle approved these changes Feb 11, 2025
View reviewed changes

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

Hide comment

I strongly suspect that this is going to break people who are testing against exact error messages, but that's really not part of our public API, so that is fine.

@donbarbos

donbarbos commented Feb 12, 2025

Copy link
Copy Markdown
Contributor Author

@pganssle CI successfully passed 👍

@pganssle pganssle merged commit 3e222e3 into python:main Feb 12, 2025
encukou added a commit to picnixz/cpython that referenced this pull request Feb 19, 2025
@encukou
Merge in the main branch to resolve conflict with python#127345
f108a14
@donbarbos donbarbos mentioned this pull request Mar 6, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ZeroIntensity ZeroIntensity ZeroIntensity left review comments

@pganssle pganssle pganssle approved these changes

@abalkin abalkin Awaiting requested review from abalkin

@erlend-aasland erlend-aasland Awaiting 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.

4 participants

@donbarbos @erlend-aasland @ZeroIntensity @pganssle