Issue38155
Created on 2019-09-13 10:23 by p-ganssle, last changed 2022-04-11 14:59 by admin. This issue is now closed.
| Pull Requests | |||
|---|---|---|---|
| URL | Status | Linked | Edit |
| PR 16203 | merged | ta1hia, 2019-09-16 21:45 | |
| Messages (10) | |||
|---|---|---|---|
| msg352280 - (view) | Author: Paul Ganssle (p-ganssle) * | Date: 2019-09-13 10:23 | |
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 (https://github.com/python/cpython/blob/6a517c674907c195660fa9178a7b561de49cc721/Lib/test/test_datetime.py#L1), where we need to import both modules anyway. I think we can accept an "add __all__" PR without tests, though. |
|||
| msg352295 - (view) | Author: Tahia K (ta1hia) * | Date: 2019-09-13 11:41 | |
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 :) |
|||
| msg352298 - (view) | Author: Paul Ganssle (p-ganssle) * | Date: 2019-09-13 11:47 | |
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. |
|||
| msg352302 - (view) | Author: Paul Ganssle (p-ganssle) * | Date: 2019-09-13 11:58 | |
Actually, how about adding this simpler test into `Lib/test/datetimetester.py`, right above test_name_cleanup (https://github.com/python/cpython/blob/ff2e18286560e981f4e09afb0d2448ea994414d8/Lib/test/datetimetester.py#L65): 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. |
|||
| msg352338 - (view) | Author: Dong-hee Na (corona10) * | Date: 2019-09-13 14:17 | |
Good news, When this issue is solved. Can we solve the issue from https://github.com/python/cpython/pull/15633 ? |
|||
| msg352458 - (view) | Author: Tahia K (ta1hia) * | Date: 2019-09-15 00:33 | |
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.
|
|||
| msg352464 - (view) | Author: Dong-hee Na (corona10) * | Date: 2019-09-15 07:18 | |
@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. |
|||
| msg352585 - (view) | Author: Tahia K (ta1hia) * | Date: 2019-09-16 21:48 | |
Thanks @corona10. I've posted a PR with all these bits. |
|||
| msg352790 - (view) | Author: Paul Ganssle (p-ganssle) * | Date: 2019-09-19 13:34 | |
New changeset 96b1c59c71534db3f0f3799cd84e2006923a5098 by Paul Ganssle (t k) in branch 'master': bpo-38155: Add __all__ to datetime module (GH-16203) https://github.com/python/cpython/commit/96b1c59c71534db3f0f3799cd84e2006923a5098 |
|||
| msg352791 - (view) | Author: Paul Ganssle (p-ganssle) * | Date: 2019-09-19 13:37 | |
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! |
|||
| History | |||
|---|---|---|---|
| Date | User | Action | Args |
| 2022-04-11 14:59:20 | admin | set | github: 82336 |
| 2019-09-19 13:37:04 | p-ganssle | set | status: open -> closed resolution: fixed messages: + msg352791 stage: patch review -> resolved |
| 2019-09-19 13:34:44 | p-ganssle | set | messages: + msg352790 |
| 2019-09-16 21:48:51 | ta1hia | set | messages: + msg352585 |
| 2019-09-16 21:45:47 | ta1hia | set | keywords:
+ patch stage: needs patch -> patch review pull_requests: + pull_request15807 |
| 2019-09-15 07:18:00 | corona10 | set | messages: + msg352464 |
| 2019-09-15 00:33:48 | ta1hia | set | messages: + msg352458 |
| 2019-09-13 14:17:00 | corona10 | set | nosy:
+ corona10 messages: + msg352338 |
| 2019-09-13 11:58:22 | p-ganssle | set | messages: + msg352302 |
| 2019-09-13 11:47:09 | p-ganssle | set | messages: + msg352298 |
| 2019-09-13 11:41:24 | ta1hia | set | nosy:
+ ta1hia messages: + msg352295 |
| 2019-09-13 10:23:10 | p-ganssle | create | |