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

Add __all__ to the datetime module #82336

Closed
pganssle opened this issue Sep 13, 2019 · 10 comments
Closed

Add __all__ to the datetime module #82336

pganssle opened this issue Sep 13, 2019 · 10 comments
Labels
3.9 only security fixes easy stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@pganssle
Copy link
Member

BPO 38155
Nosy @abalkin, @pganssle, @corona10, @tahia-khan
PRs
  • bpo-38155: Adding __all__ to datetime module #16203
  • 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 2019-09-19.13:37:04.650>
    created_at = <Date 2019-09-13.10:23:10.946>
    labels = ['easy', 'type-feature', 'library', '3.9']
    title = 'Add __all__ to the datetime module'
    updated_at = <Date 2019-09-19.13:37:04.649>
    user = 'https://github.com/pganssle'

    bugs.python.org fields:

    activity = <Date 2019-09-19.13:37:04.649>
    actor = 'p-ganssle'
    assignee = 'none'
    closed = True
    closed_date = <Date 2019-09-19.13:37:04.650>
    closer = 'p-ganssle'
    components = ['Library (Lib)']
    creation = <Date 2019-09-13.10:23:10.946>
    creator = 'p-ganssle'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 38155
    keywords = ['patch', 'newcomer friendly']
    message_count = 10.0
    messages = ['352280', '352295', '352298', '352302', '352338', '352458', '352464', '352585', '352790', '352791']
    nosy_count = 4.0
    nosy_names = ['belopolsky', 'p-ganssle', 'corona10', 'ta1hia']
    pr_nums = ['16203']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue38155'
    versions = ['Python 3.9']

    @pganssle
    Copy link
    Member Author

    Currently the datetime module has no all, which means we only advertise what is public and private based on leading underscores. Additionally, because there are two implementations (Python and C), you actually get different things when you do from datetime import * depending on whether you have the C module installed or not.

    The "easy" part is to add an __all__ variable to Lib/datetime.py for all the documented attributes:

      __all__ = ["date", "datetime", "time", "timedelta", "timezone", "tzinfo", "MINYEAR", "MAXYEAR"]

    A "stretch goal" would be to add a test to ensure that from datetime import * imports the same set of symbols from the pure python module that it does from the C module. I haven't quite thought through how this would be achieved, probably something in test_datetime (

    import unittest
    ), where we need to import both modules anyway. I think we can accept an "add all" PR without tests, though.

    @pganssle pganssle added 3.9 only security fixes stdlib Python modules in the Lib dir easy type-feature A feature request or enhancement labels Sep 13, 2019
    @tahia-khan
    Copy link
    Mannequin

    tahia-khan mannequin commented Sep 13, 2019

    Hello! I'm interested in picking up this task. Is it okay if I grab it?

    The import test bit seems tricky, since (from what I understand) inspecting on an "import *" statement might conflict against other global imports in that test file. I'm a new contributor and open to suggestions on that bit :)

    @pganssle
    Copy link
    Member Author

    Hi Tahia: Go ahead and make a PR, no need to worry about the test.

    I mainly put in the bit about tests because I was hoping to nerd-snipe someone into figuring out how to do it for me ;) It's not a particularly important test.

    @pganssle
    Copy link
    Member Author

    Actually, how about adding this simpler test into [Lib/test/datetimetester.py](https://github.com/python/cpython/blob/main/Lib/test/datetimetester.py), right above test_name_cleanup (

    def test_name_cleanup(self):
    ):

        def test_all(self):
            """Test that __all__ only points to valid attributes."""
            all_attrs = dir(datetime_module)
            for attr in datetime_module.__all__:
                self.assertIn(attr, all_attrs)

    This will at least test that __all__ only contains valid attributes on the module.

    @corona10
    Copy link
    Member

    Good news, When this issue is solved.
    Can we solve the issue from #15633 ?

    @tahia-khan
    Copy link
    Mannequin

    tahia-khan mannequin commented Sep 15, 2019

    I'll definitely add that test @paul.

    Speaking of, what do you guys think of this test:

         def test_c_all(self):
             """Test that __all__ symbols between the c datetime module and
             the python datetime library are equivalent."""
    
             c_datetime = import_fresh_module('datetime', fresh=['_datetime'])
             py_datetime = import_fresh_module('datetime', blocked=['_datetime'])
             self.assertEqual(c_datetime.__all__, py_datetime.__all__)

    I found the import_fresh_module here: https://docs.python.org/3/library/test.html#test.support.import_fresh_module - super handy! The test currently passes for me locally, but I wanted to check if my usage was correct before submitting a PR.

    @corona10
    Copy link
    Member

    @ta1hia

    My idea is that both tests should be added.
    @p-ganssle's test aims to check the attr is allowed or not.
    And your test aims to check attribute equalities of pure python module and c module.

    @tahia-khan
    Copy link
    Mannequin

    tahia-khan mannequin commented Sep 16, 2019

    Thanks @corona10. I've posted a PR with all these bits.

    @pganssle
    Copy link
    Member Author

    New changeset 96b1c59 by Paul Ganssle (t k) in branch 'master':
    bpo-38155: Add __all__ to datetime module (GH-16203)
    96b1c59

    @pganssle
    Copy link
    Member Author

    Closing this as resolved. I don't think we should backport this, as it's more of an enhancement than a bug fix (and since no one has ever complained about it to my knowledge, I don't think there's any big rush to see this released).

    Thanks Tahia and all the reviewers!

    @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.9 only security fixes easy stdlib Python modules in the Lib dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants