Skip to content

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

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

Merged
merged 31 commits into from
Feb 12, 2025

Conversation

donBarbos
Copy link
Contributor

@donBarbos donBarbos commented Nov 27, 2024

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
Copy link

bedevere-app bot commented Nov 27, 2024

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 gh-109798: Update error messages to be the same in datetime Nov 27, 2024
@erlend-aasland
Copy link
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
Copy link
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 gh-109798: Normalize _datetime and datetime error messages Nov 29, 2024
Copy link
Member

@ZeroIntensity ZeroIntensity left a comment

Choose a reason for hiding this comment

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

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.

Copy link
Member

@ZeroIntensity ZeroIntensity left a comment

Choose a reason for hiding this comment

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

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

@donBarbos
Copy link
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

Copy link
Member

@ZeroIntensity ZeroIntensity left a comment

Choose a reason for hiding this comment

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

Getting close to ready :)

Comment on lines 63 to 70
assert 1 <= month <= 12, f"month must be in 1..12, but got {month}"
return _DAYS_BEFORE_MONTH[month] + (month > 2 and _is_leap(year))

def _ymd2ord(year, month, day):
"year, month, day -> ordinal, considering 01-Jan-0001 as day 1."
assert 1 <= month <= 12, 'month must be in 1..12'
assert 1 <= month <= 12, f"month must be in 1..12, but got {month}"
dim = _days_in_month(year, month)
assert 1 <= day <= dim, ('day must be in 1..%d' % dim)
assert 1 <= day <= dim, f"day must be in 1..{dim}, but got {day}"
Copy link
Member

Choose a reason for hiding this comment

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

I think it's worth changing these to ifs and ValueErrors; assertions are disabled when Python is ran with -O. (Though, it's theoretically possible that will break code for people relying on AssertionError. I'm not sure how big of an issue that is.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the practice of calling assert is also found in other modules and I think this issue should be raised separately (not in this PR)

Copy link
Member

Choose a reason for hiding this comment

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

I think it's fine to do it in this PR, because IMO, unifying the error types fall under the category of "normalizing." I'm not super crazy about it though--any opinion @erlend-aasland?

@donBarbos
Copy link
Contributor Author

donBarbos commented Dec 20, 2024

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

@ZeroIntensity
Copy link
Member

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

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.

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.

@@ -2419,7 +2422,7 @@ def __new__(cls, offset, name=_Omitted):
if not cls._minoffset <= offset <= cls._maxoffset:
raise ValueError("offset must be a timedelta "
"strictly between -timedelta(hours=24) and "
"timedelta(hours=24).")
f"timedelta(hours=24), not {offset!r}")
Copy link
Member

Choose a reason for hiding this comment

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

Probably not going to be a great user experience here because of how bad the timedelta formatter is:

>>> print(f"{timedelta(hours=-25)!r}")
datetime.timedelta(days=-2, seconds=82800)

Maybe it will be clearer if we do it this way:

if offset < timedelta(0):
    offset_str = f"-{-offset)}"
else:
     offset_str = str(offset)

That will print stuff like -1 day, 1:00:00 instead of timedelta(days=-2, seconds=82800). Though looking at it, I realize that the way it gets printed is misleading, since that's the output you get from print(timedelta(hours=-23)) and users have no way of knowing what is going on under the hood there.

Probably a more elaborate timedelta formatter would be better, since we basically always want this in HH:MM:SS.fff format, since that reflects what we care about best and also is unambigous, but we don't have a particularly easy way to do that. I guess we can revisit this when/if #85426 is implemented.

Copy link
Contributor Author

@donBarbos donBarbos Feb 11, 2025

Choose a reason for hiding this comment

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

@pganssle
Okay, then let's solve this issue in a separate PR linked to that issue (I can send the changes soon).
I think this one can be merged for now and I would be glad if you review another PR #127242 for datetime module :)

@donBarbos
Copy link
Contributor Author

@pganssle CI successfully passed 👍

@pganssle pganssle merged commit 3e222e3 into python:main Feb 12, 2025
42 checks passed
encukou added a commit to picnixz/cpython that referenced this pull request Feb 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants