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

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

Merged
merged 8 commits into from Jan 24, 2018

Conversation

Projects
None yet
5 participants
@pganssle
Copy link
Contributor

pganssle commented Dec 28, 2017

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

This comment has been minimized.

Copy link
Contributor Author

pganssle commented Dec 28, 2017

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 pganssle:timezone_api branch 8 times, most recently from e85cfc9 to 70fe5a2 Dec 29, 2017

@abalkin

This comment has been minimized.

Copy link
Member

abalkin commented Jan 9, 2018

@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
Copy link
Member

abalkin left a comment

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

@@ -75,6 +75,18 @@ Type-check macros:
*NULL*.
.. c:function:: int PyTimeZone_Check(PyObject *ob)

This comment has been minimized.

Copy link
@abalkin

abalkin Jan 9, 2018

Member

Note that the typezone type cannot be subclassed:

>>> from datetime import *
>>> class tz(timezone):
...    pass
...
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: type 'datetime.timezone' is not an acceptable base type
:c:data:`PyDateTime_TimeZoneType`. *ob* must not be *NULL*.
.. c:function:: int PyTimeZone_CheckExact(PyObject *ob)

This comment has been minimized.

Copy link
@abalkin

abalkin Jan 9, 2018

Member

Since we are not exposing PyDateTime_TimeZone struct, I don't see the utility of having PyTimeZone_Check macros. Maybe something like PyTZInfo_Check as an accelerated version of isinstance(x, datetime.tzinfo) will be useful once we expose C versions of tzinfo.utcoffset, etc. methods, but this is better left for a separate PR.

This comment has been minimized.

Copy link
@pganssle

pganssle Jan 9, 2018

Author Contributor

Why not just remove the _Check and leave the _CheckExact? I would think that the _CheckExact function is still meaningful.

@bedevere-bot

This comment has been minimized.

Copy link

bedevere-bot commented Jan 9, 2018

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.

@abalkin abalkin self-assigned this Jan 9, 2018

@@ -154,13 +154,18 @@ typedef struct {
PyTypeObject *TimeType;
PyTypeObject *DeltaType;
PyTypeObject *TZInfoType;
PyTypeObject *TimeZoneType;

This comment has been minimized.

Copy link
@pganssle

pganssle Jan 9, 2018

Author Contributor

@abalkin If you don't want the _Check macros exposed, do you also not want this exposed?

This comment has been minimized.

Copy link
@abalkin

@pganssle pganssle force-pushed the pganssle:timezone_api branch from 70fe5a2 to d026f56 Jan 12, 2018

@pganssle

This comment has been minimized.

Copy link
Contributor Author

pganssle commented Jan 12, 2018

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

@@ -155,12 +155,16 @@ typedef struct {
PyTypeObject *DeltaType;
PyTypeObject *TZInfoType;

/* singletons */
PyObject *TimeZone_UTC;

This comment has been minimized.

Copy link
@abalkin

abalkin Jan 16, 2018

Member

Shouldn't we also add a macro definition below to simplify access to the singleton?

#ifdef Py_BUILD_CORE
 ..
#else
#define PyDateTime_TimeZone_UTC PyDateTimeAPI->TimeZone_UTC
#endif

The idea is to make code in user extensions look similar to that in _datetimemodule.c.

This comment has been minimized.

Copy link
@pganssle

pganssle Jan 16, 2018

Author Contributor

Yeah, I'm fine with that. I think I didn't know of any examples of other singletons exposed in the C API so I didn't know to do this.

@pganssle pganssle force-pushed the pganssle:timezone_api branch 2 times, most recently from fb1b06c to c38ea33 Jan 16, 2018

@@ -13,6 +13,16 @@ the module initialisation function. The macro puts a pointer to a C structure
into a static variable, :c:data:`PyDateTimeAPI`, that is used by the following
macros.

Macro for access to the UTC singleton:

This comment has been minimized.

Copy link
@pganssle

pganssle Jan 17, 2018

Author Contributor

@abalkin I had to add a new section for this and decided to put it at the top, please let me know if you'd prefer it to be somewhere else.

@vadmium You did a great review of the documentation on #4699, do you want to take a look at this?

@abalkin abalkin requested a review from vadmium Jan 17, 2018


.. c:var:: PyObject* PyDateTime_TimeZone_UTC
Returns the time zone singleton representing UTC, this returns the same

This comment has been minimized.

Copy link
@vadmium

vadmium Jan 19, 2018

Member

Remove “this returns” (or start a new sentence)

.. c:var:: PyObject* PyDateTime_TimeZone_UTC
Returns the time zone singleton representing UTC, this returns the same
object as ``datetime.timezone.utc``.

This comment has been minimized.

Copy link
@vadmium

vadmium Jan 19, 2018

Member

Can you link to :attr:`datetime.timezone.utc` here (and similarly elsewhere)? If so, I think they would be useful.

This comment has been minimized.

Copy link
@pganssle

pganssle Jan 20, 2018

Author Contributor

I don't see any other references to timezone.utc in c-api/datetime.rst, do you mean add other links to things other than timezone.utc, or modify other documents to link to timezone.utc?

.. c:function:: PyObject* PyTimeZone_FromOffset(PyDateTime_DeltaType* offset)
Return a ``datetime.timezone`` object with an unnamed fixed offset
represented by the ``offset`` argument.

This comment has been minimized.

Copy link
@vadmium

vadmium Jan 19, 2018

Member

Parameter names are normally marked up in italics as *offset*

est_capi, est_macro, est_macro_nn = _testcapi.make_timezones_capi()

exp_named = timezone(timedelta(hours=-5), "EST")
exp_unnamed = timezone(timedelta(hours=-5))

This comment has been minimized.

Copy link
@vadmium

vadmium Jan 19, 2018

Member

Looks like there is no practical difference between these two as far as the test is concerned, so I suggest to drop one of them. Comparison of datetime objects only considers the offset.

This comment has been minimized.

Copy link
@pganssle

pganssle Jan 20, 2018

Author Contributor

Ooh, good call, as written this test doesn't actually check if PyTimeZone_FromOffsetAndName actually works.

pganssle added some commits Dec 28, 2017

@pganssle pganssle force-pushed the pganssle:timezone_api branch from c38ea33 to 70642af Jan 20, 2018

resulting number of microseconds and seconds lie in the ranges documented for
:class:`datetime.timedelta` objects.
.. c:function:: PyObject* PyTimeZone_FromOffset(PyDateTime_DeltaType* offset)

This comment has been minimized.

Copy link
@pganssle

pganssle Jan 20, 2018

Author Contributor

When I build the documentation, I see Return value: New reference. under all the constructor macros except these two, but I'm not sure where that's being generated from. I don't see why they wouldn't be a new reference, but I don't 100% grok when something does and does not change the reference count.

This comment has been minimized.

Copy link
@abalkin

abalkin Jan 23, 2018

Member

Reference counting information is stored in Doc/data/refcounts.dat

This comment has been minimized.

Copy link
@pganssle

pganssle Jan 24, 2018

Author Contributor

@abalkin When documenting the reference counts, I ran into a bit of a problem, since the reference to offset is not increased if offset is equivalent to timedelta(0), so it's ambiguous whether the function's effect on offset's reference count is +1 or 0. I went with +1 and added a comment to the effect that it's not always true.

This comment has been minimized.

Copy link
@abalkin

abalkin Jan 24, 2018

Member

I am going to merge this now. We can fine tune the documentation during the beta phase.

@pganssle

This comment has been minimized.

Copy link
Contributor Author

pganssle commented Jan 21, 2018

@vadmium Should be good now.

@abalkin abalkin merged commit 04af5b1 into python:master Jan 24, 2018

4 checks passed

bedevere/issue-number Issue number 10381 found
Details
bedevere/news News entry found in Misc/NEWS.d
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@pganssle pganssle deleted the pganssle:timezone_api branch Oct 25, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.