classification
Title: Make Calendar.itermonthdates() behave consistently in edge cases
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 3.7
process
Status: closed Resolution: fixed
Dependencies: 26650 28253 Superseder:
Assigned To: belopolsky Nosy List: aeros167, belopolsky, jiangping.li, rhettinger, serhiy.storchaka, xiang.zhang
Priority: normal Keywords: patch

Created on 2016-09-28 03:01 by belopolsky, last changed 2019-08-04 20:34 by rhettinger. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 4079 merged belopolsky, 2017-10-23 03:34
PR 15113 merged rhettinger, 2019-08-04 18:58
PR 15116 merged miss-islington, 2019-08-04 20:14
Messages (18)
msg277577 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2016-09-28 03:01
The Calendar.itermonthdates() method is defined as follows:

"Return an iterator for one month. The iterator will yield datetime.date values and will always iterate through complete weeks, so it will yield dates outside the specified month."

However, for the two months at the extremes of datetime.date range, 0001-01 and 9999-12, the dates outside the specified month may not be representable as datetime.date instances.

The current implementation is inconsistent: itermonthdates(1, 1) may raise an OverflowError (see #26650), while itermonthdates(9999, 12) may yield an incomplete week (see #28253.)

This issue supersedes #26650 and #28253.
msg277584 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-09-28 05:24
Possible solutions (in any case the documentation needs changes):

1. Raise OverflowError at both ends (revert issue15421patch). The method becomes unusable for two extreme months.

2. Yield an incomplete week at both ends (apply the patch from issue26650). This can cause silent producing incorrect result in third-party programs.

3. Yield None for unrepresentable dates. This is more useful than raising OverflowError, and noisily fails, but third-party code should be changed to support extreme cases.

4. Yield dummy date instance for unrepresentable dates (apply the patch from issue28253). If we are lucky, the third-party code will just work, without any changes. If we are not lucky, this makes debugging harder.

5. Make datetime.date supporting a date outside current limits. This is a can of worms (numbering years B.D., output and parsing dates with negative and more than 4-digit years).

Examples of the usage of itermonthdates():

https://github.com/sunlightlabs/django-locksmith/blob/master/locksmith/hub/dataviews.py
https://github.com/quandyfactory/Quandy/blob/master/quandy.py
https://github.com/takanory/plone.app.event/blob/master/plone/app/event/portlets/portlet_calendar.py
https://github.com/gerow/gnome-shell-google-calendar/blob/master/gnome-shell-google-calendar.py
https://bitbucket.org/benallard/msgboard/src/1c08fa3ba040f8151d0e28130b01b30e0595e448/msgboard/controller.py?at=default
msg277644 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2016-09-28 16:26
I would like to take the #1 approach, and also implement an itermonthdays3() generator that would be like  itermonthdates(), but return 3-tuples (year, month, day) instead of datetime.date objects.

The name "itermonthdays3" is subject to discussion, but it fits with the existing "itermonthdays" and "itermonthdays2" that yield integers and 2-tuples respectively.  An alternative, but slightly longer name would be "itermonthdatetuples".

itermonthdates() can then be reimplemented as

def itermonthdates(self, year, month):
    for y, m, d in self.itermonthdays3(year, month):
        yield datetime.date(y, m, d)

with the obvious out of bounds behavior.
msg277662 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-09-28 21:23
LGTM. Maybe add also itermonthdays4() that yields 4-tuples (year, month, day, day_of_week)? I seen a code that filters out weekends from itermonthdates().
msg304735 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-10-22 08:23
Do you mind to create a pull request Alexander?
msg304775 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2017-10-23 02:10
Let me look into this.  It's been a while ...
msg304776 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2017-10-23 02:25
> 1. Raise OverflowError at both ends (revert issue15421patch). The method becomes unusable for two extreme months.

The issue 15421 patch was committed in 85710a40e7e9eab86060bedc3762ccf9ca8d26ca.
msg304777 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2017-10-23 03:40
I submitted PR 4079. Serhiy, please review the logic and if this matches what we discussed a year ago, I will add the docs and NEWS entries.
msg304930 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2017-10-24 17:17
New changeset fdd9b217c60b454ac6a82f02c8b0b551caeac88b by Alexander Belopolsky in branch 'master':
Closes bpo-28292: Implemented Calendar.itermonthdays3() and itermonthdays4(). (#4079)
https://github.com/python/cpython/commit/fdd9b217c60b454ac6a82f02c8b0b551caeac88b
msg347758 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2019-07-12 21:29
Please mark all the non-public helper functions as being private by using a leading underscore for their names.  The __all__ listing is correct but is insufficient.  Note, the module already used the leading underscore convention for the some of the non-public helper classes.

Already, we have a user confused by this:  https://twitter.com/andreportela85/status/1148956749652738049
msg347771 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2019-07-13 04:53
There are two ways to indicate that a function is not public: either make its name underscored, or add __all__ and do not include the function name in it. The purpose of __all__ is to avoid mass renaming of internal functions whis is a code churn and reduces readability. If make all internal names underscored, __all__ would be not needed.
msg347772 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2019-07-13 05:23
For people using "import calendar", the __all__ variable has no effect.  Running help(calendar) shows the functions as if they were public functions.  The module historically hid its private details with the leading underscores.  The patch in question violated that norm.  We have evidence that a real user was confused by this.  Please fix it.
msg347774 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2019-07-13 05:46
s/help/dir/
msg347810 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2019-07-13 13:06
There are hundreds or thousands of private names in other modules. Do you propose to change them all? I afraid that this will cause more harm than benefit.
msg347851 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2019-07-13 18:15
> Do you propose to change them all?

No.  But we have precedent in this module and should maintain it.  FWIW, there are a number of modules that have been conscientious in this regard (for example, collections and random).
msg347863 - (view) Author: Kyle Stanley (aeros167) * (Python triager) Date: 2019-07-14 04:00
>But we have precedent in this module and should maintain it.

In general, applying different rules to standard library modules and changing private function naming conventions on a case-by-case basis creates rather drastic inconsistency. There definitely should be a universal standard across stdlib, especially for something as fundamental as private function naming. The precedent really should not be on a per-module basis.

As someone who joined the Python development community only a month ago but was a user of the language for 5 years, the most common convention to denote privation functions was a preceding underscore. In a perfect world where simplicity is the only concern, this should be the guideline in which stdlib follow consistently across the modules. However, Serhiy makes a very valid point:

> There are hundreds or thousands of private names in other modules. Do you propose to change them all? I afraid that this will cause more harm than benefit.

__all__ has been far easier to maintain since functions can be added or removed without issue and there's no need to rename them. However, as far as I am aware, this is no clear documentation that membership on the __all__ list denotes whether or not a function is public. Also, any user using "import module" instead of "from module import *" may not be aware they are accessing a private function.

Here's a few solutions I thought of (not mutually exclusive):

1) Document that "The list from __all__ is the universal stdlib convention for marking functions as public, and any not on the list are considered private. When possible, underscore is preferred as well, but the lack of an underscore does not necessarily mean the function is public."

2) When a user attempts to import or use a function that is not in __all__, a minimally intrusive warning message is shown upon execution to notify them that they are using a private function which is not officially supported for external usage. There should be a way to easily suppress this message if the user desires.

3) Add a new syntax similar to "import module" that only imports public functions. Currently, "from module import *" somewhat provides this, but does not allow for using the "module.function" syntax for referencing the functions.

The exact convention for marking functions as public or private is not nearly as important as having universal consistency across stdlib and clear communication to the users. This does not seem to be the case at the moment.
msg349000 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2019-08-04 20:14
New changeset b1c8ec010fb4eb2654ca994e95144c8f2fea07fb by Raymond Hettinger in branch 'master':
bpo-28292: Mark calendar.py helper functions as private. (GH-15113)
https://github.com/python/cpython/commit/b1c8ec010fb4eb2654ca994e95144c8f2fea07fb
msg349002 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2019-08-04 20:34
New changeset 0c16f6b307f7607e29b98b8fbb99cbca28f91a48 by Raymond Hettinger (Miss Islington (bot)) in branch '3.8':
bpo-28292: Mark calendar.py helper functions as private. (GH-15113) (GH-15116)
https://github.com/python/cpython/commit/0c16f6b307f7607e29b98b8fbb99cbca28f91a48
History
Date User Action Args
2019-08-04 20:34:59rhettingersetmessages: + msg349002
2019-08-04 20:29:07rhettingersetstatus: open -> closed
stage: patch review -> resolved
2019-08-04 20:14:13miss-islingtonsetpull_requests: + pull_request14857
2019-08-04 20:14:06rhettingersetmessages: + msg349000
2019-08-04 18:58:56rhettingersetstage: resolved -> patch review
pull_requests: + pull_request14855
2019-07-14 04:00:17aeros167setnosy: + aeros167
messages: + msg347863
2019-07-13 18:15:01rhettingersetmessages: + msg347851
2019-07-13 13:06:45serhiy.storchakasetmessages: + msg347810
2019-07-13 05:46:38rhettingersetmessages: + msg347774
2019-07-13 05:23:08rhettingersetmessages: + msg347772
2019-07-13 04:53:53serhiy.storchakasetmessages: + msg347771
2019-07-12 21:29:52rhettingersetstatus: closed -> open

messages: + msg347758
2017-10-24 17:17:14belopolskysetstatus: open -> closed
resolution: fixed
messages: + msg304930

stage: patch review -> resolved
2017-10-23 03:40:53belopolskysetmessages: + msg304777
2017-10-23 03:34:00belopolskysetkeywords: + patch
stage: needs patch -> patch review
pull_requests: + pull_request4049
2017-10-23 02:25:32belopolskysetmessages: + msg304776
2017-10-23 02:10:31belopolskysetmessages: + msg304775
2017-10-22 08:23:04serhiy.storchakasetmessages: + msg304735
components: + Library (Lib)
stage: test needed -> needs patch
2016-09-28 21:23:39serhiy.storchakasetmessages: + msg277662
2016-09-28 16:26:28belopolskysetmessages: + msg277644
2016-09-28 05:24:06serhiy.storchakasetmessages: + msg277584
2016-09-28 03:05:06belopolskysetdependencies: + calendar: OverflowErrors for year == 1 and firstweekday > 0, calendar.prcal(9999) output has a problem
2016-09-28 03:04:31belopolskylinkissue26650 superseder
2016-09-28 03:02:52belopolskylinkissue28253 superseder
2016-09-28 03:01:58belopolskycreate