bpo-20177: Migrate datetime.date.fromtimestamp to Argument Clinic #8535

timhoffm · 2018-07-28T16:40:25Z

This PR migrates datetime.date.fromtimestamp to the Argument Clinic.

There's much more to to in bpo-20177, but it's a start to see how the conversion is going.

#europython2018-sprint

https://bugs.python.org/issue20177

the-knights-who-say-ni · 2018-07-28T16:40:27Z

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately our records indicate you have not signed the CLA. For legal reasons we need you to sign this before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

When your account is ready, please add a comment in this pull request
and a Python core developer will remove the CLA not signed label
to make the bot check again.

You can check yourself
to see if the CLA has been received.

Thanks again for your contribution, we look forward to reviewing it!

timhoffm · 2018-07-28T16:47:37Z

For the record: CLA is signed and awaits confirmation.

serhiy-storchaka · 2018-07-29T04:02:45Z

Modules/_datetimemodule.c

+@classmethod
+datetime.date.fromtimestamp
+
+    timestamp: 'O'


timestamp: object

Thanks. Didn't get to that point in the clinic documentation.

serhiy-storchaka · 2018-07-29T04:05:49Z

Modules/_datetimemodule.c

-    if (PyArg_ParseTuple(args, "O:fromtimestamp", &timestamp))
-        result = date_local_from_object(cls, timestamp);
-    return result;
+    return date_fromtimestamp((PyObject *) type, timestamp);


Wouldn't be better to just move the Argument Clinic declaration before date_fromtimestamp, and rename the later to datetime_date_fromtimestamp?

I don't think that will work because date_fromtimestamp is the C API whereas datetime_date_fromtimestamp is the Python API and they differ in the type of the first argument. But maybe I'm overlooking something? These are just my first steps with CPython.

I meant that since date_fromtimestamp is used in just a single place, why not inline it? But instead of moving the body of date_fromtimestamp inside datetime_date_fromtimestamp, it is better to move the header of datetime_date_fromtimestamp (generated by Argument Clinic, together with the prepending Argument Clinic declaration) in place of the header of date_fromtimestamp. Both datetime_date_fromtimestamp and date_fromtimestamp are C functions, with virtually identical signatures.

Sorry if I'm not clear.

You mean, I should have just one function not the two? I don't think this works for the following reason.

I have to use datetime.date.fromtimestamp for the clinic. If I just use date.fromtimestampit will complain (Parent class or module does not exist).
With the clinic declaration, this results in exactly the signature

datetime_date_fromtimestamp(PyTypeObject *type, PyObject *timestamp)

However, I cannot use this function in the PyDateTime_CAPI declaration (l.6075). If I try that, the compiler complains

/home/tim/dev/cpython/Modules/_datetimemodule.c:6075:5: warning: initialization from incompatible pointer type [-Wincompatible-pointer-types] datetime_date_fromtimestamp, ^~~~~~~~~~~~~~~~~~~~~~~~~~~ /home/tim/dev/cpython/Modules/_datetimemodule.c:6075:5: note: (near initialization for ‘CAPI.Date_FromTimestamp’)

The function there is expected to have PyObject * as type, not PyTypeObject *. I don't know if that can be corrected, but reckon that it would be a breaking change to the CAPI which we do not want.

Ah, I missed that date_fromtimestamp()is not a new function, and it is used n other place. You could cast the function to the correct type as it was in thedate_methods` initialization:

(PyCFunction)datetime_date_fromtimestamp,

but the current code LGTM too.

serhiy-storchaka · 2018-07-29T04:07:54Z

Modules/_datetimemodule.c

+
+Create a date from a POSIX timestamp.
+
+The timestamp must be a double, e.g. created via time.time() and is


"must"? Aren't integers accepted too?

Yes. I sort of just copied that from the original comment ("Python timestamp -- a double"). That's probably not reasonable anyway since this is the docstring for the Python API and thus should not use a C type. Is "The timestamp is a number, e.g. ..." ok?

Is "The timestamp is a number, e.g. ..." ok?

LGTM.

serhiy-storchaka · 2018-07-29T05:50:39Z

Misc/NEWS.d/next/Documentation/2018-07-28-17-17-42.bpo-20177.cOZJWp.rst

@@ -0,0 +1 @@
+Migrate datetime.date.fromtimestamp to Argument Clinic


Please add a sentence ending period and "Patch by yourname."

serhiy-storchaka · 2018-07-29T16:34:17Z

I thought that the _datetime module is already converted to Argument Clinic, and just a new datetime.date.fromtimestamp is left. But now I see that the only one method was converted (as an example for Argument Clinic). It is not worth to open separate PRs for every method. It would be better to convert the whole module at once. Your changes for datetime.date.fromtimestamp LGTM, but would you mind to convert also other methods? Seems in most cases it should be trivial.

timhoffm · 2018-07-29T21:28:19Z

Can do, but won't be until in a couple of days. I created this PR in the EuroPython sprint. I currently have other things to do. You can leave this open until I've added more conversions.

Also, I wanted to be sure that I'm on the right path and that's easier with a smaller PR. Generally, do you prefer less but larger PRs?

serhiy-storchaka · 2018-07-30T08:14:25Z

You are on the right path. I prefer merging simple related changes into a single PR.

timhoffm · 2018-07-30T11:43:01Z

Thanks for the clarification. I heard that you squash anyway before a merge. So I can simply add further commits for the changes?

serhiy-storchaka · 2018-07-30T15:00:36Z

Yes, you can simply add further commits for the changes.

encukou · 2018-09-10T17:08:18Z

I think we should merge this; it is an improvement after all. And it may be blocking other people from working on the other functions.
Serhiy, do you agree?

pganssle · 2018-09-23T13:54:35Z

I agree with @encukou, I think this should probably be merged and a separate issue for "migrate all date, time and datetime constructors to Argument clinic" should be opened. The fact that one has been done can serve as a useful template for the others.

In the process of converting the date.fromtimestamp function to use argument clinic in pythonGH-8535, the C API for PyDate_FromTimestamp was inadvertently changed to expect a timestamp object rather than an argument tuple. This PR fixes this backwards-incompatible change by adding a new wrapper function for the C API function that unwraps the argument tuple and passes it to the underlying function. This PR also adds tests for both PyDate_FromTimestamp and PyDateTime_FromTimestamp to prevent any further regressions. bpo-36025

In the process of converting the date.fromtimestamp function to use argument clinic in GH-8535, the C API for PyDate_FromTimestamp was inadvertently changed to expect a timestamp object rather than an argument tuple. This PR fixes this backwards-incompatible change by adding a new wrapper function for the C API function that unwraps the argument tuple and passes it to the underlying function. This PR also adds tests for both PyDate_FromTimestamp and PyDateTime_FromTimestamp to prevent any further regressions.

the-knights-who-say-ni added the CLA not signed label Jul 28, 2018

bedevere-bot added the awaiting review label Jul 28, 2018

Migrate datetime.date.fromtimestamp to Argument Clinic

59a5e57

serhiy-storchaka self-requested a review July 29, 2018 04:01

serhiy-storchaka added type-feature A feature request or enhancement and removed CLA not signed labels Jul 29, 2018

the-knights-who-say-ni added the CLA signed label Jul 29, 2018

serhiy-storchaka reviewed Jul 29, 2018

View reviewed changes

timhoffm force-pushed the some-clinic branch from e51a2cc to ed4dd8e Compare July 29, 2018 04:43

serhiy-storchaka reviewed Jul 29, 2018

View reviewed changes

timhoffm force-pushed the some-clinic branch from ed4dd8e to 59a5e57 Compare July 29, 2018 08:20

encukou added the 🤖 automerge label Sep 24, 2018

encukou merged commit a0fd7f1 into python:master Sep 24, 2018

bedevere-bot removed the awaiting review label Sep 24, 2018

larryhastings mentioned this pull request Apr 10, 2022

Derby #8: Convert 28 sites to Argument Clinic across 2 files #64376

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-20177: Migrate datetime.date.fromtimestamp to Argument Clinic #8535

bpo-20177: Migrate datetime.date.fromtimestamp to Argument Clinic #8535

timhoffm commented Jul 28, 2018 •

edited by bedevere-bot

Loading

the-knights-who-say-ni commented Jul 28, 2018

timhoffm commented Jul 28, 2018

serhiy-storchaka Jul 29, 2018

timhoffm Jul 29, 2018

serhiy-storchaka Jul 29, 2018

timhoffm Jul 29, 2018 •

edited

Loading

serhiy-storchaka Jul 29, 2018

timhoffm Jul 29, 2018 •

edited

Loading

serhiy-storchaka Jul 29, 2018

serhiy-storchaka Jul 29, 2018

timhoffm Jul 29, 2018

serhiy-storchaka Jul 29, 2018

serhiy-storchaka Jul 29, 2018

serhiy-storchaka commented Jul 29, 2018

timhoffm commented Jul 29, 2018

serhiy-storchaka commented Jul 30, 2018

timhoffm commented Jul 30, 2018

serhiy-storchaka commented Jul 30, 2018

encukou commented Sep 10, 2018

pganssle commented Sep 23, 2018


		Create a date from a POSIX timestamp.

		The timestamp must be a double, e.g. created via time.time() and is

		@@ -0,0 +1 @@
		Migrate datetime.date.fromtimestamp to Argument Clinic

bpo-20177: Migrate datetime.date.fromtimestamp to Argument Clinic #8535

bpo-20177: Migrate datetime.date.fromtimestamp to Argument Clinic #8535

Conversation

timhoffm commented Jul 28, 2018 • edited by bedevere-bot Loading

the-knights-who-say-ni commented Jul 28, 2018

timhoffm commented Jul 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timhoffm Jul 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timhoffm Jul 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

serhiy-storchaka commented Jul 29, 2018

timhoffm commented Jul 29, 2018

serhiy-storchaka commented Jul 30, 2018

timhoffm commented Jul 30, 2018

serhiy-storchaka commented Jul 30, 2018

encukou commented Sep 10, 2018

pganssle commented Sep 23, 2018

timhoffm commented Jul 28, 2018 •

edited by bedevere-bot

Loading

timhoffm Jul 29, 2018 •

edited

Loading

timhoffm Jul 29, 2018 •

edited

Loading