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

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

Closed
larryhastings opened this issue Jan 7, 2014 · 21 comments
Closed

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

larryhastings opened this issue Jan 7, 2014 · 21 comments
Labels
3.9 only security fixes extension-modules C modules in the Modules dir topic-argument-clinic type-feature A feature request or enhancement

Comments

@larryhastings
Copy link
Contributor

BPO 20177
Nosy @larryhastings, @encukou, @serhiy-storchaka, @ZackerySpytz, @hauntsaninja
PRs
  • bpo-20177: Migrate datetime.date.fromtimestamp to Argument Clinic #8535
  • gh-64376: Convert the time module to the Argument Clinic #14311
  • bpo-20177: use builtin_dir to test METH_VARARGS #14330
  • gh-64376: Use Argument Clinic for datetime.date classmethods #20208
  • Files
  • timemodule.patch
  • timemodule_rev2.patch
  • timemodule_part1_rev3.patch
  • timemodule_part2_rev3.patch
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = None
    created_at = <Date 2014-01-07.23:49:34.309>
    labels = ['extension-modules', 'type-feature', 'expert-argument-clinic', '3.9']
    title = 'Derby #8: Convert 28 sites to Argument Clinic across 2 files'
    updated_at = <Date 2020-05-19.06:28:09.667>
    user = 'https://github.com/larryhastings'

    bugs.python.org fields:

    activity = <Date 2020-05-19.06:28:09.667>
    actor = 'hauntsaninja'
    assignee = 'none'
    closed = False
    closed_date = None
    closer = None
    components = ['Extension Modules', 'Argument Clinic']
    creation = <Date 2014-01-07.23:49:34.309>
    creator = 'larry'
    dependencies = []
    files = ['33534', '33607', '33716', '33717']
    hgrepos = []
    issue_num = 20177
    keywords = ['patch']
    message_count = 21.0
    messages = ['207622', '207652', '207654', '208383', '208391', '208424', '208425', '208426', '208429', '208439', '208441', '208746', '209153', '209268', '209270', '218647', '219051', '224758', '336195', '336556', '346297']
    nosy_count = 6.0
    nosy_names = ['larry', 'petr.viktorin', 'nikratio', 'serhiy.storchaka', 'ZackerySpytz', 'hauntsaninja']
    pr_nums = ['8535', '14311', '14330', '20208']
    priority = 'normal'
    resolution = None
    stage = 'patch review'
    status = 'open'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue20177'
    versions = ['Python 3.9']

    @larryhastings
    Copy link
    Contributor Author

    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":
    http://docs.python.org/dev/howto/clinic.html

    @larryhastings larryhastings added type-bug An unexpected behavior, bug, or error extension-modules C modules in the Modules dir type-feature A feature request or enhancement and removed type-bug An unexpected behavior, bug, or error labels Jan 7, 2014
    @skrah
    Copy link
    Mannequin

    skrah mannequin commented Jan 8, 2014

    Please take out _decimal.c. It has a huge test suite that would break entirely.

    @larryhastings
    Copy link
    Contributor Author

    Absolutely. decimal is a very special case.

    Whoever takes over this issue: please ignore the part about _decimal.c.

    @larryhastings larryhastings changed the title Derby #8: Convert 52 sites to Argument Clinic across 3 files Derby #8: Convert 28 sites to Argument Clinic across 2 files Jan 8, 2014
    @nikratio
    Copy link
    Mannequin

    nikratio mannequin commented Jan 18, 2014

    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:

    PyDoc_STRVAR(strftime_doc,
    "strftime(format[, tuple]) -> string\n\
    \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\
    \n" STRFTIME_FORMAT_CODES);

    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?

    @larryhastings
    Copy link
    Contributor Author

    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.

    @nikratio
    Copy link
    Mannequin

    nikratio mannequin commented Jan 18, 2014

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

    @nikratio
    Copy link
    Mannequin

    nikratio mannequin commented Jan 18, 2014

    One more question: what's a good way to find sites that need changes? Searching for "PyArg_" finds both converted and unconverted functions...

    @larryhastings
    Copy link
    Contributor Author

    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.

    @nikratio
    Copy link
    Mannequin

    nikratio mannequin commented Jan 19, 2014

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

    @nikratio
    Copy link
    Mannequin

    nikratio mannequin commented Jan 19, 2014

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

    @larryhastings
    Copy link
    Contributor Author

    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.

    @nikratio
    Copy link
    Mannequin

    nikratio mannequin commented Jan 22, 2014

    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.

    @nikratio
    Copy link
    Mannequin

    nikratio mannequin commented Jan 25, 2014

    (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]
    time.gmtime

    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.

    @nikratio
    Copy link
    Mannequin

    nikratio mannequin commented Jan 26, 2014

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

    @nikratio
    Copy link
    Mannequin

    nikratio mannequin commented Jan 26, 2014

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

    @taleinat
    Copy link
    Contributor

    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.

    @nikratio
    Copy link
    Mannequin

    nikratio mannequin commented May 24, 2014

    Tal, I was referring to this mail: https://mail.python.org/pipermail/python-dev/2014-January/132066.html

    @larryhastings
    Copy link
    Contributor Author

    All the Derby patches should only go into trunk at this point.

    @serhiy-storchaka
    Copy link
    Member

    The modern workflow requires creating a PR on GitHub. Nikolaus, do you mind to convert your patch to a PR?

    @serhiy-storchaka serhiy-storchaka added the 3.8 only security fixes label Feb 21, 2019
    @nikratio
    Copy link
    Mannequin

    nikratio mannequin commented Feb 25, 2019

    Sorry, no. I have long lost context and interest in this.

    @ZackerySpytz
    Copy link
    Mannequin

    ZackerySpytz mannequin commented Jun 22, 2019

    I have created a PR for Modules/timemodule.c.

    @ZackerySpytz ZackerySpytz mannequin added 3.9 only security fixes and removed 3.8 only security fixes labels Jun 22, 2019
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.9 only security fixes extension-modules C modules in the Modules dir topic-argument-clinic type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants