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

Make Calendar.itermonthdates() behave consistently in edge cases #72479

Closed
abalkin opened this issue Sep 28, 2016 · 18 comments
Closed

Make Calendar.itermonthdates() behave consistently in edge cases #72479

abalkin opened this issue Sep 28, 2016 · 18 comments
Assignees
Labels
3.7 stdlib type-bug

Comments

@abalkin
Copy link
Member

@abalkin abalkin commented Sep 28, 2016

BPO 28292
Nosy @rhettinger, @abalkin, @serhiy-storchaka, @zhangyangyu, @lijp_003@163.com, @aeros
PRs
  • #4079
  • #15113
  • #15116
  • Dependencies
  • bpo-26650: calendar: OverflowErrors for year == 1 and firstweekday > 0
  • bpo-28253: calendar.prcal(9999) output has a problem
  • 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 = 'https://github.com/abalkin'
    closed_at = <Date 2019-08-04.20:29:07.374>
    created_at = <Date 2016-09-28.03:01:58.463>
    labels = ['3.7', 'type-bug', 'library']
    title = 'Make Calendar.itermonthdates() behave consistently in edge cases'
    updated_at = <Date 2019-08-04.20:34:59.803>
    user = 'https://github.com/abalkin'

    bugs.python.org fields:

    activity = <Date 2019-08-04.20:34:59.803>
    actor = 'rhettinger'
    assignee = 'belopolsky'
    closed = True
    closed_date = <Date 2019-08-04.20:29:07.374>
    closer = 'rhettinger'
    components = ['Library (Lib)']
    creation = <Date 2016-09-28.03:01:58.463>
    creator = 'belopolsky'
    dependencies = ['26650', '28253']
    files = []
    hgrepos = []
    issue_num = 28292
    keywords = ['patch']
    message_count = 18.0
    messages = ['277577', '277584', '277644', '277662', '304735', '304775', '304776', '304777', '304930', '347758', '347771', '347772', '347774', '347810', '347851', '347863', '349000', '349002']
    nosy_count = 6.0
    nosy_names = ['rhettinger', 'belopolsky', 'serhiy.storchaka', 'xiang.zhang', 'jiangping.li', 'aeros']
    pr_nums = ['4079', '15113', '15116']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue28292'
    versions = ['Python 3.7']

    @abalkin
    Copy link
    Member Author

    @abalkin abalkin commented Sep 28, 2016

    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 bpo-26650), while itermonthdates(9999, 12) may yield an incomplete week (see bpo-28253.)

    This issue supersedes bpo-26650 and bpo-28253.

    @abalkin abalkin added the 3.7 label Sep 28, 2016
    @abalkin abalkin self-assigned this Sep 28, 2016
    @abalkin abalkin added the type-bug label Sep 28, 2016
    @serhiy-storchaka
    Copy link
    Member

    @serhiy-storchaka serhiy-storchaka commented Sep 28, 2016

    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 bpo-26650). 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 bpo-28253). 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

    @abalkin
    Copy link
    Member Author

    @abalkin abalkin commented Sep 28, 2016

    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.

    @serhiy-storchaka
    Copy link
    Member

    @serhiy-storchaka serhiy-storchaka commented Sep 28, 2016

    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().

    @serhiy-storchaka
    Copy link
    Member

    @serhiy-storchaka serhiy-storchaka commented Oct 22, 2017

    Do you mind to create a pull request Alexander?

    @serhiy-storchaka serhiy-storchaka added the stdlib label Oct 22, 2017
    @abalkin
    Copy link
    Member Author

    @abalkin abalkin commented Oct 23, 2017

    Let me look into this. It's been a while ...

    @abalkin
    Copy link
    Member Author

    @abalkin abalkin commented Oct 23, 2017

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

    The bpo-15421 patch was committed in 85710a4.

    @abalkin
    Copy link
    Member Author

    @abalkin abalkin commented Oct 23, 2017

    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.

    @abalkin
    Copy link
    Member Author

    @abalkin abalkin commented Oct 24, 2017

    New changeset fdd9b21 by Alexander Belopolsky in branch 'master':
    Closes bpo-28292: Implemented Calendar.itermonthdays3() and itermonthdays4(). (bpo-4079)
    fdd9b21

    @abalkin abalkin closed this as completed Oct 24, 2017
    @rhettinger
    Copy link
    Contributor

    @rhettinger rhettinger commented Jul 12, 2019

    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

    @rhettinger rhettinger reopened this Jul 12, 2019
    @serhiy-storchaka
    Copy link
    Member

    @serhiy-storchaka serhiy-storchaka commented Jul 13, 2019

    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.

    @rhettinger
    Copy link
    Contributor

    @rhettinger rhettinger commented Jul 13, 2019

    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.

    @rhettinger
    Copy link
    Contributor

    @rhettinger rhettinger commented Jul 13, 2019

    s/help/dir/

    @serhiy-storchaka
    Copy link
    Member

    @serhiy-storchaka serhiy-storchaka commented Jul 13, 2019

    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.

    @rhettinger
    Copy link
    Contributor

    @rhettinger rhettinger commented Jul 13, 2019

    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).

    @aeros
    Copy link
    Member

    @aeros aeros commented Jul 14, 2019

    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.

    @rhettinger
    Copy link
    Contributor

    @rhettinger rhettinger commented Aug 4, 2019

    New changeset b1c8ec0 by Raymond Hettinger in branch 'master':
    bpo-28292: Mark calendar.py helper functions as private. (GH-15113)
    b1c8ec0

    @rhettinger
    Copy link
    Contributor

    @rhettinger rhettinger commented Aug 4, 2019

    New changeset 0c16f6b by Raymond Hettinger (Miss Islington (bot)) in branch '3.8':
    bpo-28292: Mark calendar.py helper functions as private. (GH-15113) (GH-15116)
    0c16f6b

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.7 stdlib type-bug
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants