◐ Shell
reader mode source ↗
Skip to content

bpo-10381: Add timezone to datetime C API#5032

Merged
abalkin merged 8 commits into
python:masterfrom
pganssle:timezone_api
Jan 24, 2018
Merged

bpo-10381: Add timezone to datetime C API#5032
abalkin merged 8 commits into
python:masterfrom
pganssle:timezone_api

Conversation

@pganssle

@pganssle pganssle commented Dec 28, 2017

Copy link
Copy Markdown
Member

This adds C API access to timezone and the timezone.UTC singleton, per bpo-10381.

@abalkin I didn't totally understand what you meant by "I am not sure whether PyDateTime_TimeZone details should be exposed in datetime.h because presumably programmers should access it through the abstract tzinfo interface.", so please clarify if this PR is exposing the details you referred to.

https://bugs.python.org/issue10381

@pganssle

pganssle commented Dec 28, 2017

Copy link
Copy Markdown
Member Author

Hm. There's some sort of build error about levels of indirection that seems to only be happening on Windows. I'm not really sure what to do with that - I can take a look at it later, but if any Windows users know if that's a common thing please do let me know. This was some sort of symbol confusion because in my test I had an object named timezone.

I added documentation for the macros, but I didn't see an obvious place to document that the UTC singleton is now available through the C API.

@pganssle pganssle force-pushed the timezone_api branch 8 times, most recently from e85cfc9 to 70fe5a2 Compare January 5, 2018 17:34
@abalkin

abalkin commented Jan 9, 2018

Copy link
Copy Markdown
Member

@pganssle - IIRC, my comment on the issue was about PyDateTime_TimeZone struct which should probably stay private and not exposed in the header file. Your PR does the right thing in this regard.

@abalkin abalkin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hide comment

Please remove the "Check" methods and limit this PR to TimeZone_UTC and constructors.

@bedevere-bot

Copy link
Copy Markdown

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.

@pganssle

Copy link
Copy Markdown
Member Author

@abalkin Ready for review. I added a new commit rather than rewriting the history, because I figure it might be useful to have access to the tests and such later if this is ever exposed.

@pganssle pganssle force-pushed the timezone_api branch 2 times, most recently from fb1b06c to c38ea33 Compare January 16, 2018 21:37
17 hidden items Load more…
@pganssle

Copy link
Copy Markdown
Member Author

@vadmium Should be good now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants