Skip to content
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

gh-126476: Raise IllegalMonthError for calendar.formatmonth method when the input month is not corret #126484

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No proper validation for month in Calendar Module
6 participants