Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

gh-109798: Normalize_datetime anddatetime 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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
pganssle merged 31 commits intopython:mainfromdonBarbos:issue-109798
Feb 12, 2025

Conversation

donBarbos
Copy link
Contributor

@donBarbosdonBarbos commentedNov 27, 2024
edited
Loading

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

ValueError: ('year must be in 1..9999', {year})ValueError:Yearisoutofrange: {year}ValueError:year {year}isoutofrangeValueError:Year {year}isoutofrange

i made this one error:

ValueError:yearmustbein1..9999,not {year}

and I made the same message template for the fieldsyear,month,day,hour,minute,second,microsecond andfold 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

Most changes to Pythonrequire a NEWS entry. Add one using theblurb_it web app or theblurb command-line tool.

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

@donBarbosdonBarbos changed the titleUpdate error messages to be the same indatetimegh-109798: Update error messages to be the same indatetimeNov 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 alsothe devguide.

@donBarbos
Copy link
ContributorAuthor

@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 reacted with thumbs up emoji

@erlend-aaslanderlend-aasland changed the titlegh-109798: Update error messages to be the same indatetimegh-109798: Normalize_datetime anddatetime error messagesNov 29, 2024
donBarbosand others added6 commitsNovember 29, 2024 13:30
Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
Copy link
Member

@ZeroIntensityZeroIntensity 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, butPyErr_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.

donBarbos and erlend-aasland reacted with thumbs up emoji
donBarbosand others added6 commitsNovember 30, 2024 04:17
Co-authored-by: Peter Bierma <zintensitydev@gmail.com>
Co-authored-by: Peter Bierma <zintensitydev@gmail.com>
Co-authored-by: Peter Bierma <zintensitydev@gmail.com>
Co-authored-by: Peter Bierma <zintensitydev@gmail.com>
Co-authored-by: Peter Bierma <zintensitydev@gmail.com>
Copy link
Member

@ZeroIntensityZeroIntensity 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 someassertRaisesRegex tests 😄

donBarbos reacted with thumbs up emojierlend-aasland reacted with laugh emoji
@donBarbos
Copy link
ContributorAuthor

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

ZeroIntensity reacted with thumbs up emoji

Copy link
Member

@ZeroIntensityZeroIntensity 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}"

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 toifs andValueErrors; assertions are disabled when Python is ran with-O. (Though, it's theoretically possible that will break code for people relying onAssertionError. I'm not sure how big of an issue that is.)

Copy link
ContributorAuthor

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?

donBarbosand others added4 commitsDecember 1, 2024 07:43
Co-authored-by: Peter Bierma <zintensitydev@gmail.com>
Co-authored-by: Peter Bierma <zintensitydev@gmail.com>
@donBarbos
Copy link
ContributorAuthor

donBarbos commentedDec 20, 2024
edited
Loading

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

donBarbos reacted with thumbs up emoji

Copy link
Member

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

donBarbos and srinivasreddy reacted with rocket emoji
@@ -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 thetimedelta 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:

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

That will print stuff like-1 day, 1:00:00 instead oftimedelta(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 fromprint(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 inHH: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
ContributorAuthor

@donBarbosdonBarbosFeb 11, 2025
edited
Loading

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 fordatetime module :)

@donBarbos
Copy link
ContributorAuthor

@pganssle CI successfully passed 👍

srinivasreddy reacted with hooray emojisrinivasreddy reacted with heart emoji

@pgansslepganssle merged commit3e222e3 intopython:mainFeb 12, 2025
42 checks passed
encukou added a commit to picnixz/cpython that referenced this pull requestFeb 19, 2025
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@ZeroIntensityZeroIntensityZeroIntensity left review comments

@pgansslepgansslepganssle approved these changes

@abalkinabalkinAwaiting requested review from abalkinabalkin is a code owner

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

4 participants
@donBarbos@erlend-aasland@ZeroIntensity@pganssle

[8]ページ先頭

©2009-2025 Movatter.jp