calendar.nextmonth and calendar.prevmonth functions doesn't check if the month is valid

[Asocia]

BPO	35406
Nosy	@rhettinger, @abalkin, @serhiy-storchaka, @srinivasreddy, @pganssle, @asocia
PRs	bpo-35406: Add validity check to prevmonth and nextmonth functions #10890

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2018-12-04.19:18:30.843>
created_at = <Date 2018-12-04.10:55:26.343>
labels = ['library']
title = "calendar.nextmonth and calendar.prevmonth functions doesn't check if the month is valid"
updated_at = <Date 2018-12-04.19:18:30.841>
user = 'https://github.com/Asocia'

bugs.python.org fields:

activity = <Date 2018-12-04.19:18:30.841>
actor = 'asocia'
assignee = 'none'
closed = True
closed_date = <Date 2018-12-04.19:18:30.843>
closer = 'asocia'
components = ['Library (Lib)']
creation = <Date 2018-12-04.10:55:26.343>
creator = 'asocia'
dependencies = []
files = []
hgrepos = []
issue_num = 35406
keywords = ['patch']
message_count = 12.0
messages = ['331031', '331032', '331035', '331037', '331042', '331046', '331050', '331052', '331053', '331054', '331057', '331072']
nosy_count = 6.0
nosy_names = ['rhettinger', 'belopolsky', 'serhiy.storchaka', 'thatiparthy', 'p-ganssle', 'asocia']
pr_nums = ['10890']
priority = 'normal'
resolution = None
stage = 'resolved'
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue35406'
versions = []

[Asocia]

import calendar
calendar.nextmonth(2018, 11) returns (2018, 12) which is OK.
calendar.nextmonth(2018, 12) returns (2019, 1) which is also OK.
calendar.nextmonth(2018, 13) returns (2018, 14). It would make more sense if this was raise calendar.IllegalMonthError.

[serhiy-storchaka]

prevmonth() and nextmonth() are internal functions. They are used only in itermonthdays3(), and the month is already checked before calling them.

[Asocia]

I understand you but i still think these functions need to check it. As an end-user, I shouldn't see these functions to work with no errors for illegal months.

[serhiy-storchaka]

What is the problem with current code? Can you provide an example that doesn't work correctly?

[Asocia]

I'm suggesting this idea to consistency. Why an IllegalMonthError exists in calendar module if we don't raise this error when required?

What would you say if I asked you to "What is the month number coming after 156th month?" Would you say 157 or prefer to inform me that I'm doing something wrong?

>>> import calendar
>>> calendar.nextmonth(2018, 12)
(2019, 1)

If Python is smart enough to jump next year's first month and not say (2018, 13) blindly, it should also check if the given month is valid.

But:
>>> calendar.nextmonth(2018, 157)
(2018, 158)

I think this is clearly a bug in the code.

---------------------------------------------------

I'll wander away from the this issue but some of the functions in calendar module also not consistent with each other:

>> calendar.monthcalendar(2018, 12) # Runs with no problem.
>> calendar.monthcalendar(2018, 0) # Raises IllegalMonthError.
>> calendar.monthcalendar(2018, 13) # Raises IllegalMonthError.

>> calendar.month(2018, 12) # Runs with no problem.
>> calendar.month(2018, 0) # Raises IllegalMonthError.
>> calendar.month(2018, 13) # Raises IndexError???

Why? Wouldn't it be more reasonable if the last one also had raised IllegalMonthError?

>> calendar.monthrange(2018, 12) # Runs with no problem.
>> calendar.monthrange(2018, 0) # Raises IllegalMonthError.
>> calendar.monthrange(2018, 13) # Raises IllegalMonthError.

>> calendar.prmonth(2018, 12) # Runs with no problem.
>> calendar.prmonth(2018, 0) # Raises IllegalMonthError.
>> calendar.prmonth(2018, 13) # Raises IndexError.

[serhiy-storchaka]

nextmonth() is not a public API. You should not use it.

If you want to make IllegalMonthError be always raised instead of IndexError for out of range month values, this is a different issue.

[srinivasreddy]

I agree with Serhiy, nextmonth() is not a public API,you should not use it.

[Asocia]

OK now it isn't a problem if we shouldn't use this function directly but how am i going to understand if a function is public API or not? In classes, we are using single or double underscore to indicate that the function or variable we're declaring is intended to be private. Is there anything similar to this for "public API functions" or am I in the wrong way?

[abalkin]

On Dec 4, 2018, at 10:27 AM, Şahin <report@bugs.python.org> wrote:

Is there anything similar to this for "public API functions"?

Yes - read the reference manual. If the function is not there it is not public.

[pganssle]

Might it be worth moving nextmonth and prevmonth to calendar._nextmonth and calendar._prevmonth to make it more clear that these are private methods?

Due to Hyrum's Law, people will be using them anyway, but it could have a short deprecation period where calendar.nextmonth and calendar.prevmonth raise DeprecationWarning and then call calendar._nextmonth and calendar._prevmonth.

[serhiy-storchaka]

They are not in the __all__ list and are not documented. If __all__ is defined for the module, there is no need to use the underscore prefix for private globals. The calendar module is not special, many other modules follow this convention.

[Asocia]

OK, thank you all for information. It's now clear for me too why this is not an issue. I'll try to close this issue by selecting status as close but if it isn't working with this way (I'm new, I don't know how) anyone can close this.