bpo-10381: Add timezone to datetime C API by pganssle · Pull Request #5032

bpo-10381: Add timezone to datetime C API by pganssle · Pull Request #5032 · python/cpython

Conversation

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

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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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.

Choose a reason for hiding this comment

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

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

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.

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

yes

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

Choose a reason for hiding this comment

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

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.

Choose a reason for hiding this comment

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

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.

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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?

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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.

Choose a reason for hiding this comment

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

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

		resulting number of microseconds and seconds lie in the ranges documented for
		:class:`datetime.timedelta` objects.

		.. c:function:: PyObject* PyTimeZone_FromOffset(PyDateTime_DeltaType* offset)

Choose a reason for hiding this comment

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

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.

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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