Skip to content

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

Closed
Closed
Derby #8: Convert 28 sites to Argument Clinic across 2 files#64376
Labels
3.9 (EOL)end of lifeextension-modulesC modules in the Modules dirtopic-argument-clinictype-featureA feature request or enhancement

Description

@larryhastings
larryhastings
opened on Jan 7, 2014
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']

    Activity

    1. larryhastings commented on Jan 7, 2014

      @larryhastings
      larryhastings
      on Jan 7, 2014
      ContributorAuthor

      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

    2. larryhastings
      added
      type-bugAn unexpected behavior, bug, or error
      extension-modulesC modules in the Modules dir
      type-featureA feature request or enhancement
      and removed
      type-bugAn unexpected behavior, bug, or error
      on Jan 7, 2014

    3. skrah commented on Jan 8, 2014

      @skrah
      skrahmannequin
      on Jan 8, 2014
      Mannequin

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

    4. larryhastings commented on Jan 8, 2014

      @larryhastings
      larryhastings
      on Jan 8, 2014
      ContributorAuthor

      Absolutely. decimal is a very special case.

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

    5. 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[/+] on Jan 8, 2014

    6. nikratio commented on Jan 18, 2014

      @nikratio
      nikratiomannequin
      on Jan 18, 2014
      Mannequin

      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?

    7. larryhastings commented on Jan 18, 2014

      @larryhastings
      larryhastings
      on Jan 18, 2014
      ContributorAuthor

      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.

    8. nikratio commented on Jan 18, 2014

      @nikratio
      nikratiomannequin
      on Jan 18, 2014
      Mannequin

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

    9. nikratio commented on Jan 18, 2014

      @nikratio
      nikratiomannequin
      on Jan 18, 2014
      Mannequin

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

    10. larryhastings commented on Jan 18, 2014

      @larryhastings
      larryhastings
      on Jan 18, 2014
      ContributorAuthor

      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.

    11. nikratio commented on Jan 19, 2014

      @nikratio
      nikratiomannequin
      on Jan 19, 2014
      Mannequin

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

    12. nikratio commented on Jan 19, 2014

      @nikratio
      nikratiomannequin
      on Jan 19, 2014
      Mannequin

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

    13. 2 remaining items

    14. nikratio commented on Jan 25, 2014

      @nikratio
      nikratiomannequin
      on Jan 25, 2014
      Mannequin

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

    15. nikratio commented on Jan 26, 2014

      @nikratio
      nikratiomannequin
      on Jan 26, 2014
      Mannequin

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

    16. nikratio commented on Jan 26, 2014

      @nikratio
      nikratiomannequin
      on Jan 26, 2014
      Mannequin

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

    17. taleinat commented on May 16, 2014

      @taleinat
      taleinat
      on May 16, 2014
      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.

    18. nikratio commented on May 24, 2014

      @nikratio
      nikratiomannequin
      on May 24, 2014
      Mannequin

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

    19. larryhastings commented on Aug 4, 2014

      @larryhastings
      larryhastings
      on Aug 4, 2014
      ContributorAuthor

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

    20. serhiy-storchaka
      added
      topic-argument-clinic
      on Feb 25, 2015

    21. serhiy-storchaka commented on Feb 21, 2019

      @serhiy-storchaka
      serhiy-storchaka
      on Feb 21, 2019
      Member

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

    22. serhiy-storchaka
      added
      3.8 (EOL)end of life
      on Feb 21, 2019

    23. nikratio commented on Feb 25, 2019

      @nikratio
      nikratiomannequin
      on Feb 25, 2019
      Mannequin

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

    24. ZackerySpytz commented on Jun 22, 2019

      @ZackerySpytz
      ZackerySpytzmannequin
      on Jun 22, 2019
      Mannequin

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

    25. ZackerySpytz
      added
      3.9 (EOL)end of life
      and removed
      3.8 (EOL)end of life
      on Jun 22, 2019
    26. ezio-melotti
      transferred this issue fromon Apr 10, 2022
    27. carljm
      closed this as completedon Feb 20, 2023
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Metadata

    Metadata

    Assignees

    No one assigned

      Labels

      3.9 (EOL)end of lifeextension-modulesC modules in the Modules dirtopic-argument-clinictype-featureA feature request or enhancement

      Projects

      No projects

        Milestone

        No milestone

        Relationships

        None yet

        Development

        No branches or pull requests

        Issue actions