Title: Derby #8: Convert 28 sites to Argument Clinic across 2 files
Type: enhancement Stage: needs patch
Components: Argument Clinic, Extension Modules Versions: Python 3.5
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: larry, nikratio, taleinat
Priority: normal Keywords: patch

Created on 2014-01-07 23:49 by larry, last changed 2015-02-25 15:27 by serhiy.storchaka.

File name Uploaded Description Edit
timemodule.patch nikratio, 2014-01-19 01:03 review
timemodule_rev2.patch nikratio, 2014-01-22 04:39 review
timemodule_part1_rev3.patch nikratio, 2014-01-26 02:08 review
timemodule_part2_rev3.patch nikratio, 2014-01-26 02:14
Messages (18)
msg207622 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-01-07 23:49
This issue is part of the Great Argument Clinic Conversion Derby,
where we're trying to convert as much of Python 3.4 to use
Argument Clinic as we can before Release Candidate 1 on January 19.

This issue asks you to change the following bundle of files:
    Modules/_datetimemodule.c: 20 sites
    Modules/timemodule.c: 8 sites
    Modules/_decimal/_decimal.c: 24 sites

Talk to me (larry) if you only want to attack part of a bundle.

For instructions on how to convert a function to work with Argument
Clinic, read the "howto":
msg207652 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2014-01-08 00:55
Please take out _decimal.c.  It has a huge test suite that would break entirely.
msg207654 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-01-08 01:04
Absolutely.  decimal is a very special case.

Whoever takes over this issue: please ignore the part about _decimal.c.
msg208383 - (view) Author: Nikolaus Rath (nikratio) * Date: 2014-01-18 04:47
I have started working on this (partial patch attached), but I am unsure how to handle cases where the docstring definition utilizes a macro. For example:

"strftime(format[, tuple]) -> string\n\
Convert a time tuple to a string according to a format specification.\n\
See the library reference manual for formatting codes. When the time tuple\n\
is not present, current time as returned by localtime() is used.\n\

I don't think STRFTIME_FORMAT_CODES will be expanded if I put it into the [clinic] comment section. 

Should I include the contents verbatim? Or do not convert these functions to argument clinic?
msg208391 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-01-18 09:12
That macro, STRFTIME_FORMAT_CODES, is only used in two functions.  If it were me I'd just copy and paste the text into both functions.
msg208424 - (view) Author: Nikolaus Rath (nikratio) * Date: 2014-01-18 23:17
Will do. Another question: what should I do with return annotations, e.g. the "-> dict" part in

"get_clock_info(name: str) -> dict\n

Should I drop that part? In the clinic howto, I couldn't find anything about return annotations (only that parameter annotations are unsupported).
msg208425 - (view) Author: Nikolaus Rath (nikratio) * Date: 2014-01-18 23:29
One more question: what's a good way to find sites that need changes? Searching for "PyArg_" finds both converted and unconverted functions...
msg208426 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-01-18 23:54
Yes, you should drop things that look like return annotations in the original docstring.

I don't have a magic formula for finding unchanged functions, sorry.
msg208429 - (view) Author: Nikolaus Rath (nikratio) * Date: 2014-01-19 01:03
Here is a patch for timemodule.c. I have converted everything except: 

* ctime() uses the gettmarg() function. Converting gettmarg() itself doesn't seem possible to me (because it's not Python callable), and duplicating the functionality in a single complicated clinic section just for ctime() did not seem reasonable either.

* gmtime(), localtime(), ctime() use a the parse_time_t_args() utility function. I have not yet figured out how to replace this with argument clinic (sent a mail to python-dev about that).
msg208439 - (view) Author: Nikolaus Rath (nikratio) * Date: 2014-01-19 03:58
Hmm. After reading some of the threads on python-dev, I'm now confused if I did the conversion of optional arguments correctly.

Should something like "strftime(format[, tuple])" be converted using optional groups, or should tuple become a parameter with a default value? (so far, I have done the latter).
msg208441 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-01-19 04:14
Optional groups really are only for cases when that's the behavior
of the original code.  If the original function used a switch statement
examining the size of the tuple, then had different parsing functions based on that size, use optional groups.

But if the function can be expressed with normal positional parameters and default values, that's best.
msg208746 - (view) Author: Nikolaus Rath (nikratio) * Date: 2014-01-22 04:39
As discussed on devel, here's an updated patch for timemodule.c that
uses a custom C converter to handle the parse_time_t_args() utility
function. The only function I did not convert is mktime, because I did
not find a way to do so without duplicating the gettmarg() utility
function (which cannot be converted because it is also called by other
C functions).

You probably want to take a close look at what I did to the strptime
function. I didn't see a way to include the actual default value
(rather than None) in the signature without reconstructing the
argument tuple for calling the Python implementation.

As far as I can see, this completes the conversion for timemodule.c. I'll
work on _datetimemodule.c next, but I'm not sure I'll have time before
next weekend.

This is probably already planned, but since I haven't seen it in
Misc/NEWS yet: I think it would be a good idea to entry informing the
user about the conversion from strictly optional parameters to
parameters with default values. Eg.:

 * Issues xxx, yyy, zzz: many functions in the standard library now
   accept `None` for optional arguments. Previously, specifying `None`
   mostly resulted in a `TypeError`. Starting with this version,
   passing `None` is equivalent to not specfying the parameter.
msg209153 - (view) Author: Nikolaus Rath (nikratio) * Date: 2014-01-25 06:07
(I already sent this to python-dev, but maybe it makes more sense to have these thoughts together with the patch)

After looking at the conversion of parse_time_t_args again, I think I lost track of what we're actually gaining with this procedure. If all we need is a type that can distinguish between a valid time_t value and a default value, why don't we simply use PyObject?

In other words, what's the advantage of the extra custom strict, plus the custom C converter function, plus the custom converter python class over:

static int
PyObject_to_time_t(PyObject *obj, time_t *when)
     if (obj == NULL || obj == Py_None) 
         *when = time(NULL);
     else if (_PyTime_ObjectToTime_t(obj, when) == -1)
         return 0;
     return 1;

/*[clinic input]

    seconds: object=NULL

static PyObject *
time_gmtime_impl(PyModuleDef *module, PyObject *seconds)
    PyObject *return_value = NULL;
    time_t when;
    if(!PyObj_to_time_t(seconds, &when))
       return NULL;


To me this version looks shorter and clearer.
msg209268 - (view) Author: Nikolaus Rath (nikratio) * Date: 2014-01-26 01:55
I've attached updated patches to reflect the recent changes in the derby policy.

part1 is the part of the patch that is suitable for 3.4.

part2 are the changes that have to wait for 3.5 (mostly because of default values).
msg209270 - (view) Author: Nikolaus Rath (nikratio) * Date: 2014-01-26 02:27
I'll wait with working on _datetimemodule.c until someone had a chance to look over my _timemodule.c patch. (I want to make sure I'm following the right procedure).
msg218647 - (view) Author: Tal Einat (taleinat) * (Python committer) Date: 2014-05-16 08:07
Regarding the handling of time_t arguments which can be None, I agree that the second version (without custom convertors) is simpler and clearer while having no disadvantage that I can see.

I'd like to review the rest of the patches, but you mention changes to the AC policy regarding 3.4 and 3.5 and I can't find a reference to these changes or to the new policy.
msg219051 - (view) Author: Nikolaus Rath (nikratio) * Date: 2014-05-24 19:33
Tal, I was referring to this mail:
msg224758 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-08-04 20:12
All the Derby patches should only go into trunk at this point.
Date User Action Args
2015-02-25 15:27:40serhiy.storchakasetcomponents: + Argument Clinic
2014-08-04 20:12:59larrysetmessages: + msg224758
versions: + Python 3.5, - Python 3.4
2014-05-24 19:33:24nikratiosetmessages: + msg219051
2014-05-16 09:03:06skrahsetnosy: - skrah
2014-05-16 08:07:25taleinatsetnosy: + taleinat
messages: + msg218647
2014-01-26 02:27:05nikratiosetmessages: + msg209270
2014-01-26 02:14:48nikratiosetfiles: + timemodule_part2_rev3.patch
2014-01-26 02:08:59nikratiosetfiles: + timemodule_part1_rev3.patch
2014-01-26 02:08:53nikratiosetfiles: - timemodule_part1_rev3.patch
2014-01-26 01:55:57nikratiosetfiles: + timemodule_part1_rev3.patch

messages: + msg209268
2014-01-25 06:07:20nikratiosetmessages: + msg209153
2014-01-22 04:39:38nikratiosetfiles: + timemodule_rev2.patch

messages: + msg208746
2014-01-19 04:14:29larrysetmessages: + msg208441
2014-01-19 03:58:19nikratiosetmessages: + msg208439
2014-01-19 01:03:29nikratiosetfiles: - issue20177.patch
2014-01-19 01:03:17nikratiosetfiles: + timemodule.patch

messages: + msg208429
2014-01-18 23:54:46larrysetmessages: + msg208426
2014-01-18 23:29:36nikratiosetmessages: + msg208425
2014-01-18 23:17:48nikratiosetmessages: + msg208424
2014-01-18 09:12:51larrysetmessages: + msg208391
2014-01-18 04:47:16nikratiosetfiles: + issue20177.patch

nosy: + nikratio
messages: + msg208383

keywords: + patch
2014-01-08 01:36:37r.david.murraylinkissue20187 dependencies
2014-01-08 01:04:41larrysetmessages: + msg207654
title: Derby #8: Convert 52 sites to Argument Clinic across 3 files -> Derby #8: Convert 28 sites to Argument Clinic across 2 files
2014-01-08 00:55:27skrahsetnosy: + skrah
messages: + msg207652
2014-01-07 23:52:51larrysettype: behavior -> enhancement
2014-01-07 23:49:34larrycreate