Skip to content

Conversation

@Zheaoli
Copy link
Contributor

@Zheaoli Zheaoli commented Nov 6, 2024

@ericvsmith
Copy link
Member

I'm not sure this is a good idea. If someone was catching IndexError previously, then the change to IllegalMonthError will break their code.

@ZeroIntensity
Copy link
Member

Yeah, Eric is right. I would be ok with this if IllegalMonthError also inherited from IndexError.

@Zheaoli
Copy link
Contributor Author

Zheaoli commented Nov 7, 2024

I have update this PR,PTAL @ericvsmith @ZeroIntensity

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.

I'm not sure how to feel about this. It works, and the implementation looks fine, but it's also a little hacky so I'm hesitant to approve. I'll wait for Eric's input.

@ericvsmith
Copy link
Member

Yeah, I'm not crazy about it. It seems like there must be other places where we've done something similar, though. Does anyone feel like searching the stdlib for some place else we've done this?

@tomasr8
Copy link
Member

tomasr8 commented Nov 9, 2024

I didn't find any examples of errors inheriting from IndexError, but there is precedent for changing IndexError to something else. Here are two examples that I found:

https://github.com/python/cpython/pull/8184/files#diff-654faea1665f8db34a06e155e3f168b467e2071967404ea9692f41190fcbb5d4

https://github.com/python/cpython/pull/123613/files#diff-f6ee3a81bee278bb507d0cba22bb56acb204667c1271fc5d3fc41aa702248c35

@ericvsmith
Copy link
Member

I was thinking more along the lines of using multiple inheritance for exceptions. But those are good examples of changing the type of an exception.

@tomasr8
Copy link
Member

tomasr8 commented Nov 9, 2024

I was thinking more along the lines of using multiple inheritance for exceptions.

Ah sorry, I misunderstood. I checked again and found two examples of multiple inheritance:

class MultipartConversionError(MessageError, TypeError):
    """Conversion to a multipart is prohibited."""


class UnsupportedOperation(OSError, ValueError):
        pass

Copy link
Member

@ethanfurman ethanfurman left a comment

Choose a reason for hiding this comment

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

Multiple inheritance for exceptions is a common technique (at least in my own code 😄 ). Looks good to me.

Copy link
Contributor

@hauntsaninja hauntsaninja left a comment

Choose a reason for hiding this comment

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

Thanks Tomas for finding examples! I'd vote that it's probably okay to change the exception type. Adding IndexError as a base can break code too if someone has an except IndexError for some other reason and calls monthrange.

(side note: the UnsupportedOperation Tomas mentions has finicky history)

@ethanfurman ethanfurman merged commit 3be7498 into python:main Nov 15, 2024
36 checks passed
@ethanfurman ethanfurman added type-bug An unexpected behavior, bug, or error stdlib Standard Library Python modules in the Lib/ directory 3.12 only security fixes 3.13 bugs and security fixes needs backport to 3.12 only security fixes 3.14 bugs and security fixes needs backport to 3.13 bugs and security fixes labels Nov 15, 2024
@miss-islington-app
Copy link

Thanks @Zheaoli for the PR, and @ethanfurman for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

@miss-islington-app
Copy link

Thanks @Zheaoli for the PR, and @ethanfurman for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Nov 15, 2024
…hen the input month is not correct (pythonGH-126484)

(cherry picked from commit 3be7498)

Co-authored-by: Nadeshiko Manju <[email protected]>
Co-authored-by: Ethan Furman <[email protected]>
@bedevere-app
Copy link

bedevere-app bot commented Nov 15, 2024

GH-126878 is a backport of this pull request to the 3.12 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.12 only security fixes label Nov 15, 2024
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Nov 15, 2024
…hen the input month is not correct (pythonGH-126484)

(cherry picked from commit 3be7498)

Co-authored-by: Nadeshiko Manju <[email protected]>
Co-authored-by: Ethan Furman <[email protected]>
@bedevere-app
Copy link

bedevere-app bot commented Nov 15, 2024

GH-126879 is a backport of this pull request to the 3.13 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label Nov 15, 2024
ethanfurman added a commit that referenced this pull request Nov 15, 2024
…when the input month is not correct (GH-126484) (GH-126878)

gh-126476: Raise IllegalMonthError for calendar.formatmonth() when the input month is not correct (GH-126484)
(cherry picked from commit 3be7498)

Co-authored-by: Nadeshiko Manju <[email protected]>
Co-authored-by: Ethan Furman <[email protected]>
ethanfurman added a commit that referenced this pull request Nov 15, 2024
…when the input month is not correct (GH-126484) (GH-126879)

gh-126476: Raise IllegalMonthError for calendar.formatmonth() when the input month is not correct (GH-126484)
(cherry picked from commit 3be7498)

Co-authored-by: Nadeshiko Manju <[email protected]>
Co-authored-by: Ethan Furman <[email protected]>
@Zheaoli Zheaoli deleted the manjusaka/fix-126476 branch November 17, 2024 14:39
picnixz pushed a commit to picnixz/cpython that referenced this pull request Dec 8, 2024
…hen the input month is not correct (pythonGH-126484)

Co-authored-by: Ethan Furman <[email protected]>
ebonnal pushed a commit to ebonnal/cpython that referenced this pull request Jan 12, 2025
…hen the input month is not correct (pythonGH-126484)

Co-authored-by: Ethan Furman <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3.12 only security fixes 3.13 bugs and security fixes 3.14 bugs and security fixes stdlib Standard Library Python modules in the Lib/ directory type-bug An unexpected behavior, bug, or error

Projects

None yet

Development

Successfully merging this pull request may close these issues.

No proper validation for month in Calendar Module

6 participants