bpo-38155: Adding __all__ to datetime module by ta1hia · Pull Request #16203 · python/cpython

ta1hia · 2019-09-16T21:45:46Z

Co-Authored-By: Paul Ganssle [email protected]

https://bugs.python.org/issue38155

ammaraskar · 2019-09-16T22:51:42Z

Hmm, I think there's a simpler way to test this, just do:

from datetime import *

This will throw an AttributeError if you have an invalid attribute defined there.

Example (test.py):

__all__ = ['a'] b = 1

>>> from test import * Traceback (most recent call last): File "<stdin>", line 1, in <module> AttributeError: module 'test' has no attribute 'a'

cc @pganssle

@ammaraskar from x import * is only valid at the top level of a file, so this cannot be executed in a test.

@pganssle Ah, is this already the most viable way to test the validity of the attributes then?

Well, it's the best one I could think of. There may be better tests for __all__, but given that this module already does some funky and apparently fragile stuff with the import mechanism, I think it's probably best not to try to get too fancy - particularly since it's not that important of a test.

aeros

Thanks for the PR @tahia-khan.

I have some suggestions regarding the news entry, but overall +1 on the PR. This seems to be well aligned with what @pganssle was asking for in the issue comments.

aeros · 2019-09-17T04:36:54Z

Made a couple of minor formatting adjustments and removed the part about the tests. News entries typically don't contain information about the tests, as they aren't part of the public API.

Suggested change

Add __all__ to the datetime module, including unit tests.

Add ``__all__`` to :mod:`datetime`.

@tahia-khan You may also want to (optionally) add your name to the news - "Patch by Tahia Khan." (or whatever you prefer to be credited as).

corona10 · 2019-09-17T04:38:53Z

Please make sure it follows to limit all lines to a maximum of 79 characters.
PEP8: https://www.python.org/dev/peps/pep-0008/#maximum-line-length

In case this isn't clear, this can easily be fixed by changing it to:

Suggested change

__all__ = ("date", "datetime", "time", "timedelta", "timezone", "tzinfo", "MINYEAR", "MAXYEAR")

__all__ = ("date", "datetime", "time", "timedelta", "timezone", "tzinfo",

"MINYEAR", "MAXYEAR")

aeros · 2019-09-17T04:45:00Z

This section is also going over the 79 char limit. Please adjust it accordingly.

For this I'll go with parenthesis notation from the PEP328 rather than backslashes.

I think you won't need to change this line at all once you have dropped test_c_all.

pganssle

Thanks for the PR @tahia-khan, and thanks for the reviews @aeros167 and @ammaraskar! With the formatting changes and dropping test_c_all, I think this looks pretty good.

I was mildly taken aback that you've defined __all__ as a tuple rather than a list, which is traditional, but I don't see any reason why it shouldn't be a list, and in fact __all__ being immutable makes more sense than mutable, so I'm not sure why it's not more common.

pganssle · 2019-09-17T10:56:57Z

I do not think this is the right place for this test - datetimetester.py is executed twice - once with the C module and once with the Python module. This may also mess with the state of sys.modules, see this comment.

I think just drop the test_c_all test and just leave the other test.

pganssle · 2019-09-17T10:58:46Z

@tahia-khan You may also want to (optionally) add your name to the news - "Patch by Tahia Khan." (or whatever you prefer to be credited as).

bedevere-bot · 2019-09-17T11:01:55Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

ta1hia · 2019-09-17T13:22:17Z

Thanks everybody for the detailed change suggestions. I'll wait a bit for the final reviewer and submit all changes in one commit.

I was mildly taken aback that you've defined __all__ as a tuple rather than a list, which is traditional, but I don't see any reason why it shouldn't be a list, and in fact __all__ being immutable makes more sense than mutable, so I'm not sure why it's not more common.

@pganssle Ah - I didn't actually realize lists were the usual choice. I was peeking around other files that had an __all__ defined and it seemed like both lists and tuples were being used, so I went with the immutable choice :)

pganssle · 2019-09-17T13:26:03Z

Ah - I didn't actually realize lists were the usual choice. I was peeking around other files that had an __all__ defined and it seemed like both lists and tuples were being used, so I went with the immutable choice :)

Yes, I agree that you made the sensible choice here. I tried looking it up and I could see no one arguing that you shouldn't use a tuple, and some people arguing that you shouldn't use a list, so I say we keep it as tuple.

ta1hia · 2019-09-18T19:35:22Z

I have made the requested changes; please review again

bedevere-bot · 2019-09-18T19:35:25Z

Thanks for making the requested changes!

@pganssle: please review the changes made to this pull request.

Co-Authored-By: Paul Ganssle <[email protected]>

)

pganssle

Thanks everyone, this looks good, so I've rebased it and I'm going to merge now.

The alignment in the latest version also looked a little off for whatever reason, I aligned the second line in the __all__ to the parenthesis as part of the rebase.

ta1hia requested review from abalkin and pganssle as code owners September 16, 2019 21:45

the-knights-who-say-ni added the CLA signed label Sep 16, 2019

bedevere-bot added the awaiting review label Sep 16, 2019

ammaraskar reviewed Sep 16, 2019

View reviewed changes

aeros added the type-feature A feature request or enhancement label Sep 16, 2019

aeros reviewed Sep 17, 2019

View reviewed changes

aeros suggested changes Sep 17, 2019

View reviewed changes

bedevere-bot added awaiting core review and removed awaiting review labels Sep 17, 2019

corona10 requested changes Sep 17, 2019

View reviewed changes

aeros reviewed Sep 17, 2019

View reviewed changes

pganssle requested changes Sep 17, 2019

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting core review labels Sep 17, 2019

pganssle requested review from pganssle and removed request for abalkin September 17, 2019 13:26

bedevere-bot added awaiting change review and removed awaiting changes labels Sep 18, 2019

ta1hia and others added 3 commits September 19, 2019 11:00

bpo-38155: Adding __all__ to datetime module

9e99d43

Co-Authored-By: Paul Ganssle <[email protected]>

📜🤖 Added by blurb_it.

397a7a8

bpo-38155: Minor formatting changes based on PR comments (pythonGH-16203

fa23d66

)

pganssle force-pushed the bpo-38155 branch from eb3526a to fa23d66 Compare September 19, 2019 10:02

pganssle approved these changes Sep 19, 2019

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Sep 19, 2019

pganssle closed this Sep 19, 2019

pganssle reopened this Sep 19, 2019

pganssle merged commit 96b1c59 into python:master Sep 19, 2019

bedevere-bot removed the awaiting merge label Sep 19, 2019

corona10 mentioned this pull request Sep 19, 2019

bpo-24416: Return a IsoCalendarDate from date.isocalendar() #15633

Closed

	Add __all__ to the datetime module, including unit tests.
	Add ``__all__`` to :mod:`datetime`.

	__all__ = ("date", "datetime", "time", "timedelta", "timezone", "tzinfo", "MINYEAR", "MAXYEAR")
	__all__ = ("date", "datetime", "time", "timedelta", "timezone", "tzinfo",
	"MINYEAR", "MAXYEAR")

Uh oh!

Conversation

ta1hia commented Sep 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ammaraskar Sep 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aeros left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pganssle left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Sep 17, 2019

Uh oh!

ta1hia commented Sep 17, 2019

Uh oh!

pganssle commented Sep 17, 2019

Uh oh!

ta1hia commented Sep 18, 2019

Uh oh!

bedevere-bot commented Sep 18, 2019

Uh oh!

pganssle left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

ta1hia commented Sep 16, 2019 •

edited

Loading

ammaraskar Sep 16, 2019 •

edited

Loading