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

Add missing datetime interfaces #4128

Merged
merged 32 commits into from May 2, 2021
Merged

Conversation

Bluenix2
Copy link
Contributor

See Is Includes/cpython/datetime.pxd incomplete?

I used the other code in that file as a reference point, but as I did that I noticed how it doesn't actually fully conform to CPython's datetime.h file, for example see the following:

cdef inline object datetime_new(int year, int month, int day, int hour, int minute, int second, int microsecond, object tz):
return PyDateTimeAPI.DateTime_FromDateAndTime(year, month, day, hour, minute, second, microsecond, tz, PyDateTimeAPI.DateTimeType)
Compared to CPython's .h file:
https://github.com/python/cpython/blob/49fdf118aeda891401d638ac32296c7d55d54678/Include/datetime.h#L226-L228

I could widen the scope of this pull request to include making the necessary changes for that.

For the additions I have done so far, am I supposed to add tests for them?

@scoder
Copy link
Contributor

scoder commented Apr 18, 2021

Tests should go into tests/run/datetime_pxd.pyx.

@Bluenix2
Copy link
Contributor Author

Bluenix2 commented Apr 18, 2021

Perfect, I'll take a look at that and add accompaning tests. Do you have an answer on why the .pxd file seems out-of-date, can I add the missing functionality?

@scoder
Copy link
Contributor

scoder commented Apr 18, 2021

Do you have an answer to how the .pyx file seems out of date

Do you mean the .pxd file?

Note that the .pxd contains convenience functions. It only loosely tries to match the C-API functions. E.g., why not allow passing a time zone object if there is an argument for it?

If there's something missing, feel free to add it, similar to what is there currently.

@Bluenix2
Copy link
Contributor Author

Bluenix2 commented Apr 18, 2021

Note that the .pxd contains convenience functions. It only loosely tries to match the C-API functions. E.g., why not allow passing a time zone object if there is an argument for it?

Why is it not the goal to extend everything, any reasoning? Is it just that no one has done it yet?

If there's something missing, feel free to add it, similar to what is there currently.

There seems to be some "legacy" type of code for Python 2, is this to be removed too while I might be at it?

@scoder
Copy link
Contributor

scoder commented Apr 18, 2021

There seems to be some "legacy" type of code for Python 2, is this to be removed too while I might be at it?

No. We still support Py2.7 for some time to come.

@scoder
Copy link
Contributor

scoder commented Apr 18, 2021

Why is it not the goal to extend everything, any reasoning? Is it just that no one has done it yet?

Not sure I understand. If you find something missing, add it.

@scoder
Copy link
Contributor

scoder commented Apr 18, 2021

Why is it not the goal to extend everything, any reasoning? Is it just that no one has done it yet?

Not sure I understand. If you find something missing, add it.

Note that the "convenience functions" are designed to be used from Cython code. Cython has optional arguments, C doesn't. Passing NULL for an object reference is trivial in C but ugly in Cython. That's the main reasons why we are not simply copying the C-API functions/macros as they are.

@Bluenix2
Copy link
Contributor Author

Bluenix2 commented Apr 18, 2021

Not sure I understand. If you find something missing, add it.

Perfect, I'll add the missing ...AndFold functions.

Note that the "convenience functions" are designed to be used from Cython code. Cython has optional arguments, C doesn't. Passing NULL for an object reference is trivial in C but ugly in Cython. That's the main reasons why we are not simply copying the C-API functions/macros as they are.

Ah, got it!

@Bluenix2
Copy link
Contributor Author

Okay just fixed some issues 👍

One thing I just can't figure out is how to do the PyDateTimeAPI.TimeZone_UTC without getting compiler errors. For example the current commit causes:

# Time zone singleton for UTC.
cdef object utc_tz = PyDateTimeAPI.TimeZone_UTC
                                 ^
------------------------------------------------------------

datetime.pxd:153:34: Cannot convert 'PyObject *' to Python object

I got roughly the same error when trying to make it a function (get_utc()).

@da-woods
Copy link
Contributor

da-woods commented Apr 20, 2021

You probably just need a cast to object:

cdef object utc_tz = <object>PyDateTimeAPI.TimeZone_UTC

(Cython doesn't know itself that they're the same type)

Cython/Includes/cpython/datetime.pxd Outdated Show resolved Hide resolved
Cython/Includes/cpython/datetime.pxd Outdated Show resolved Hide resolved
Cython/Includes/cpython/datetime.pxd Outdated Show resolved Hide resolved
@scoder
Copy link
Contributor

scoder commented Apr 20, 2021

Cython doesn't know itself that they're the same type

PyObject* and object are NOT the same type. The first is a plain C pointer, the second is a reference counted object reference. They just look the same in C. A semantic switch from one to the other requires an explicit cast in Cython.

@Bluenix2
Copy link
Contributor Author

Bluenix2 commented Apr 22, 2021

I just commited a few things

  • Reordered the file to follow CPython's .h file in order (could be discussed)
  • Add similar properties to time and date classes as seen in datetime
  • Fix the macro mayhem using nested ifs (still missing for some constructors, not sure how I am going to do that yet)

@scoder
Copy link
Contributor

scoder commented Apr 23, 2021

Reordered the file to follow CPython's .h file in order

This unnecessarily degrades the VCS history and makes the changes difficult to review. Please revert this part.

@Bluenix2
Copy link
Contributor Author

Bluenix2 commented Apr 23, 2021

This unnecessarily degrades the VCS history and makes the changes difficult to review. Please revert this part.

Understandable, I did find the source file to become much easier to read. But it's probably not worth the cost.

Copy link
Contributor

@scoder scoder left a comment

Choose a reason for hiding this comment

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

Tests would have found most of the issues here.

Cython/Includes/cpython/datetime.pxd Show resolved Hide resolved
Cython/Includes/cpython/datetime.pxd Outdated Show resolved Hide resolved
Cython/Includes/cpython/datetime.pxd Outdated Show resolved Hide resolved
Cython/Includes/cpython/datetime.pxd Outdated Show resolved Hide resolved
Cython/Includes/cpython/datetime.pxd Outdated Show resolved Hide resolved
Cython/Includes/cpython/datetime.pxd Outdated Show resolved Hide resolved
Cython/Includes/cpython/datetime.pxd Outdated Show resolved Hide resolved
Cython/Includes/cpython/datetime.pxd Outdated Show resolved Hide resolved
@scoder
Copy link
Contributor

scoder commented Apr 25, 2021 via email

@Bluenix2
Copy link
Contributor Author

Bluenix2 commented Apr 25, 2021

For backporting the fold feature, what should be the behaviour on earlier versions. Always return 0, or set the macro to -1 so that we can throw RuntimeError?

Additionally, how am I supposed to write tests that are expected to error on one language, but do something else on another? Is there a more integrated system than catching it and returning True if it's a version where it should?

@scoder
Copy link
Contributor

scoder commented Apr 25, 2021

For backporting the fold feature, what should be the behaviour on earlier versions. Always return 0, or set the macro to -1 so that we can throw RuntimeError?

Since there is no such feature in earlier versions, I'd suggest ignoring the argument there and considering it 0 for backporting purposes. Most users could probably live with that, and those who care will have to find a way to get the required functionality backported somehow (or require a not so old CPython version).

Additionally, how am I supposed to write tests that are expected to error on one language, but do something else on another? Is there a more integrated system than catching it and returning True if it's a version where it should?

What we commonly do is either write the doctest in a way that tests sys.version_info and just succeeds for unsupported versions, or we set up a module-level __doc__ docstring and add tests to it depending on the runtime version. Doctests are just Python code.

@scoder
Copy link
Contributor

scoder commented Apr 27, 2021

even though it's not defined in the datetime.h file?

No, you can't. The object struct is not available to the C compiler.

However, to me, it looks like it's not actually used anywhere and thus doesn't need to be available. Besides, instances would probably be singletons and their C-level fields don't seem very useful to me. Certainly not for time-critical usage. Just use object.

@Bluenix2
Copy link
Contributor Author

Bluenix2 commented Apr 27, 2021

A big issue I still find is that when we use the C macros we lose the ability to pass a timezone because of how the Python macro is defined like this:

#define PyTime_FromTimeAndFold(hour, minute, second, usecond, fold) \
    PyDateTimeAPI->Time_FromTimeAndFold(hour, minute, second, usecond, \
        Py_None, fold, PyDateTimeAPI->TimeType)

Would it be bad design if we redefine that macro? I am thinking something like this:

    /* Patch Python 3.6+ so that we can pass tz */
    #ifdef PyDateTime_FromDateAndTimeAndFold
        #undef PyDateTime_FromDateAndTimeAndFold
        #define PyDateTime_FromDateAndTimeAndFold(year, month, day, hour, minute, second, usecond, tz, fold) \
            PyDateTimeAPI->Time_FromTimeAndFold(hour, minute, second, usecond, tz, fold, PyDateTimeAPI->TimeType)
    #endif

    /* Backport for Python < 3.6 */
    #ifndef PyDateTime_FromDateAndTimeAndFold
        #define PyDateTime_FromDateAndTimeAndFold(year, month, day, hour, minute, second, usecond, tz, fold) \
            PyDateTime_FromDateAndTime(year, month, day, hour, minute, second, usecond)
    #endif

At that point it may be confusing, perhaps it would be better to define it under a new name like CythonDateTime_FromDateAndTimeAndFold (maybe a shorter name :/).

@scoder
Copy link
Contributor

scoder commented Apr 27, 2021

If you need to define a macro for internal usage, the usual naming pattern is __Pyx_….

@scoder
Copy link
Contributor

scoder commented Apr 27, 2021

If you need to define a macro for internal usage, the usual naming pattern is _Pyx….

OTOH, if you really just define a macro in order to call it from an inline function, maybe you can just define the whole function as a C macro and give it a properly usable name?

Hmm, no, that would lead to naming collisions in C space because Cython could no longer mangle the cimported function names. Bad idea.

One test would only pass if ran in Sweden
tests/run/datetime_pxd.pyx Outdated Show resolved Hide resolved
@scoder
Copy link
Contributor

scoder commented May 2, 2021

We can't set DateTimeType as a return type?

I meant to use datetime, date and time as explicit return types. They are already defined as cdef types with their inline properties.

tests/run/datetime_pxd.pyx Outdated Show resolved Hide resolved
Cython/Includes/cpython/datetime.pxd Outdated Show resolved Hide resolved
Cython/Includes/cpython/datetime.pxd Outdated Show resolved Hide resolved
Cython/Includes/cpython/datetime.pxd Outdated Show resolved Hide resolved
Cython/Includes/cpython/datetime.pxd Outdated Show resolved Hide resolved
tests/run/datetime_members.pyx Show resolved Hide resolved
tests/run/datetime_members.pyx Show resolved Hide resolved
tests/run/datetime_members.pyx Show resolved Hide resolved
Cython/Includes/cpython/datetime.pxd Outdated Show resolved Hide resolved
Cython/Includes/cpython/datetime.pxd Outdated Show resolved Hide resolved
Co-authored-by: scoder <stefan_ml@behnel.de>
tests/run/datetime_pxd.pyx Outdated Show resolved Hide resolved
Cython/Includes/cpython/datetime.pxd Outdated Show resolved Hide resolved
Cython/Includes/cpython/datetime.pxd Outdated Show resolved Hide resolved
@scoder
Copy link
Contributor

scoder commented May 2, 2021

Finally! Let's get this merged, before it breaks again…

@scoder scoder merged commit 1455951 into cython:master May 2, 2021
@scoder
Copy link
Contributor

scoder commented May 2, 2021

Thanks!

@Bluenix2 Bluenix2 deleted the fill-in-datetime branch May 2, 2021 16:58
@Bluenix2
Copy link
Contributor Author

Bluenix2 commented May 2, 2021

Finally! Let's get this merged, before it breaks again…

😂 Yay!

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.

None yet

3 participants