Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Nov 15, 2024
Merged

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

merged 5 commits into from
Nov 15, 2024

Conversation

Zheaoli
Copy link
Contributor

@Zheaoli Zheaoli commented Nov 6, 2024 •
edited by bedevere-app bot
Loading

@bedevere-app bedevere-app bot mentioned this pull request Nov 6, 2024
Closed
@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

ZeroIntensity
ZeroIntensity reviewed Nov 7, 2024
View reviewed changes
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

ethanfurman
ethanfurman approved these changes Nov 12, 2024
View reviewed changes
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.

hauntsaninja
hauntsaninja reviewed Nov 14, 2024
View reviewed changes
Copy link
Contributor

@hauntsaninja hauntsaninja left a comment •
edited
Loading

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 Python modules in the Lib dir 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
@Zheaoli @ethanfurman @miss-islington
pythongh-126476: Raise IllegalMonthError for calendar.formatmonth() w…
6c8eb67 
…hen the input month is not correct (pythonGH-126484)

(cherry picked from commit 3be7498)

Co-authored-by: Nadeshiko Manju <me@manjusaka.me>
Co-authored-by: Ethan Furman <ethan@stoneleaf.us>
@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
@Zheaoli @ethanfurman @miss-islington
pythongh-126476: Raise IllegalMonthError for calendar.formatmonth() w…
0a7afb9 
…hen the input month is not correct (pythonGH-126484)

(cherry picked from commit 3be7498)

Co-authored-by: Nadeshiko Manju <me@manjusaka.me>
Co-authored-by: Ethan Furman <ethan@stoneleaf.us>
@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
@miss-islington @Zheaoli @ethanfurman
[3.12] gh-126476: Raise IllegalMonthError for calendar.formatmonth() …
cdc1dff 
…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 <me@manjusaka.me>
Co-authored-by: Ethan Furman <ethan@stoneleaf.us>
ethanfurman added a commit that referenced this pull request Nov 15, 2024
@miss-islington @Zheaoli @ethanfurman
[3.13] gh-126476: Raise IllegalMonthError for calendar.formatmonth() …
d74f1f4 
…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 <me@manjusaka.me>
Co-authored-by: Ethan Furman <ethan@stoneleaf.us>
@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
@Zheaoli @ethanfurman @picnixz
pythongh-126476: Raise IllegalMonthError for calendar.formatmonth() w…
f64c343 
…hen the input month is not correct (pythonGH-126484)

Co-authored-by: Ethan Furman <ethan@stoneleaf.us>
ebonnal pushed a commit to ebonnal/cpython that referenced this pull request Jan 12, 2025
@Zheaoli @ethanfurman @ebonnal
pythongh-126476: Raise IllegalMonthError for calendar.formatmonth() w…
2201ffb 
…hen the input month is not correct (pythonGH-126484)

Co-authored-by: Ethan Furman <ethan@stoneleaf.us>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ethanfurman ethanfurman ethanfurman approved these changes

@hauntsaninja hauntsaninja hauntsaninja left review comments

@ZeroIntensity ZeroIntensity ZeroIntensity left review comments

Assignees
No one assigned
Labels
3.12 only security fixes 3.13 bugs and security fixes 3.14 bugs and security fixes stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

No proper validation for month in Calendar Module
6 participants
@Zheaoli @ericvsmith @ZeroIntensity @tomasr8 @ethanfurman @hauntsaninja