Skip to content

Derby #4: Convert 53 sites to Argument Clinic across 5 files #64372

Closed
Closed
Derby #4: Convert 53 sites to Argument Clinic across 5 files#64372
Labels
extension-modulesC modules in the Modules dirtopic-argument-clinictype-featureA feature request or enhancement

Description

@larryhastings
larryhastings
opened on Jan 7, 2014
BPO 20173
Nosy @loewis, @larryhastings, @tiran, @zware, @serhiy-storchaka, @vajrasky
Files
  • clinic_sha1module.patch
  • clinic_sha1module_v2.patch
  • clinic_sha1module_v3.patch
  • clinic_sha256module.patch
  • clinic_sha512module.patch
  • clinic_md5module.patch
  • clinic_codecsmodule.patch
  • clinic_sha1module_v4.patch
  • clinic_sha256module_v2.patch
  • clinic_sha512module_v2.patch
  • clinic_md5module_v2.patch
  • clinic_codecsmodule_v2.patch
  • clinic_sha1module_v5.patch
  • clinic_sha256module_v3.patch
  • clinic_sha512module_v3.patch
  • clinic_md5module_v3.patch
  • issue20173_conglomerate.patch
  • codecs_clinic.patch
  • codecs_clinic_2.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 = <Date 2016-09-22.05:07:15.075>
created_at = <Date 2014-01-07.23:46:33.338>
labels = ['extension-modules', 'type-feature', 'expert-argument-clinic']
title = 'Derby #4: Convert 53 sites to Argument Clinic across 5 files'
updated_at = <Date 2016-09-22.05:07:15.072>
user = 'https://github.com/larryhastings'

    bugs.python.org fields:

    activity = <Date 2016-09-22.05:07:15.072>
actor = 'zach.ware'
assignee = 'none'
closed = True
closed_date = <Date 2016-09-22.05:07:15.075>
closer = 'zach.ware'
components = ['Extension Modules', 'Argument Clinic']
creation = <Date 2014-01-07.23:46:33.338>
creator = 'larry'
dependencies = []
files = ['33364', '33374', '33400', '33401', '33402', '33403', '33445', '33646', '33649', '33650', '33652', '33677', '33737', '33739', '33742', '33743', '33895', '38838', '39071']
hgrepos = []
issue_num = 20173
keywords = ['patch']
message_count = 38.0
messages = ['207618', '207679', '207680', '207690', '207691', '207732', '207823', '207828', '207829', '207831', '207833', '207836', '207837', '207838', '208021', '208248', '208249', '208888', '208895', '208902', '208907', '209046', '209392', '209403', '209415', '209417', '209420', '210185', '224122', '224123', '224124', '224755', '240120', '241227', '242952', '242953', '242959', '277189']
nosy_count = 7.0
nosy_names = ['loewis', 'larry', 'christian.heimes', 'python-dev', 'zach.ware', 'serhiy.storchaka', 'vajrasky']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue20173'
versions = ['Python 3.5']

    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/_codecsmodule.c: 43 sites
      Modules/sha1module.c: 2 sites
      Modules/sha256module.c: 3 sites
      Modules/sha512module.c: 3 sites
      Modules/md5module.c: 2 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. vajrasky commented on Jan 8, 2014

      @vajrasky
      vajraskymannequin
      on Jan 8, 2014
      Mannequin

      Okay, here is my first attempt. I only worked on one file (Modules/sha1module.c). I need to see whether I hit the mark or not. If yes, I can do the other files as well. Anyway, handling the keyword argument was kinda tough. There was no example so I had to shoot in the dark.

    4. larryhastings commented on Jan 8, 2014

      @larryhastings
      larryhastings
      on Jan 8, 2014
      ContributorAuthor

      Just one comment on your patch. The documentation already tells you how to handle keyword arguments (section 8 tells you how to handle default values, section 9 tells you how to handle | in the format string). If you have any suggestions on how I could improve the documentation I'd be happy to try it.

    5. vajrasky commented on Jan 8, 2014

      @vajrasky
      vajraskymannequin
      on Jan 8, 2014
      Mannequin

      An example how to convert keyword argument would be very helpful. I searched the example from existing code but nothing shows up.

    6. larryhastings commented on Jan 8, 2014

      @larryhastings
      larryhastings
      on Jan 8, 2014
      ContributorAuthor

      To be precise: a "keyword argument" is something that happens on the caller side. What you're talking about is a "positional-or-keyword parameter". And parameters are positional-or-keyword by default.

    7. vajrasky commented on Jan 9, 2014

      @vajrasky
      vajraskymannequin
      on Jan 9, 2014
      Mannequin

      Okay, this is my second attempt. I want to get METH_VARGS but I always get METH_O for positional parameters. Is there any way to circumvent this? The documentation does not cover this.

    8. larryhastings commented on Jan 10, 2014

      @larryhastings
      larryhastings
      on Jan 10, 2014
      ContributorAuthor

      That METH_O is working perfectly. You seem to be confused by it.

      The original code was kind of dumb. The function only takes two parameters: self, and a second "obj" parameter which can be any kind of object. CPython has special support for exactly this kind of function, which is METH_O. This will actually be slightly faster. In any case, that part of your conversion was correct. Don't sweat it.

      http://docs.python.org/3.4/c-api/structures.html?highlight=meth_o#METH_O

    9. vajrasky commented on Jan 10, 2014

      @vajrasky
      vajraskymannequin
      on Jan 10, 2014
      Mannequin

      But METH_O makes the update method does not work.

      >>> _sha1.sha1().update(b'a')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
SystemError: new style getargs format but argument is not a tuple

      But if you change METH_O to METH_VARARGS, it works again. I'll chip Christian Heimes. Maybe he has something to say about this.

      Other than that, I need to update my patch because currently with my patch, the sha1 constructor accepts None value. The default behaviour is to reject None value. So I need to use advanced feature of clinic for this purpose.

      I think I can convert all sha modules (including md5) in this weekend. However, _codecsmodule is totally different beast. 43 sites.... >.<

    10. larryhastings commented on Jan 10, 2014

      @larryhastings
      larryhastings
      on Jan 10, 2014
      ContributorAuthor

      Right, it doesn't work because you left the PyArg_ParseTuple call in your impl function. Remove that and it should work.

      Rule 1: Argument Clinic handles all argument parsing for you. Your "impl" function should never call PyArg_ParseTuple or PyArg_ParseTupleAndKeywords.
      Rule 2: Never modify the output of Argument Clinic by hand.

    11. vajrasky commented on Jan 10, 2014

      @vajrasky
      vajraskymannequin
      on Jan 10, 2014
      Mannequin

      This is the current behaviour of sha1 constructor. It rejects None value.

      >>> import _sha1
>>> _sha1.sha1()
<_sha1.sha1 object at 0x7f7fa7f0dea0>
>>> _sha1.sha1(None)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: object supporting the buffer API required
>>> _sha1.sha1(string=None)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: object supporting the buffer API required

      Then when I clinic it, what about the doc? This doesn't seem right.

      +PyDoc_STRVAR(sha1_SHA1_sha1__doc_,
      +"sha1(string=None)\n"
      "Return a new SHA1 hash object; optionally initialized with a string.");

    12. vajrasky commented on Jan 10, 2014

      @vajrasky
      vajraskymannequin
      on Jan 10, 2014
      Mannequin

      Rule 1: Argument Clinic handles all argument parsing for you. Your "impl" function should never call PyArg_ParseTuple or PyArg_ParseTupleAndKeywords.

      Ah, got it. Here is the third patch. Everything works fine except for the issue whether sha1 constructor should accept None value or not. Right now, after clinic, it accepts it. So _sha1.sha1() is same as _sha1.sha1(string=None).

    13. 15 remaining items

    14. vajrasky commented on Jan 27, 2014

      @vajrasky
      vajraskymannequin
      on Jan 27, 2014
      Mannequin

      Status:
      Modules/sha1module.c: done
      Modules/sha256module.c: done
      Modules/sha512module.c: done
      Modules/md5module.c: done

      All of them are ready for Python 3.4.

      /* AC 3.5: optional positional arguments */
      Modules/_codecsmodule

    15. vajrasky commented on Feb 4, 2014

      @vajrasky
      vajraskymannequin
      on Feb 4, 2014
      Mannequin

      Here is the updated patch after Larry's commit to clinic. Everything is included except codecsmodule.

    16. python-dev commented on Jul 27, 2014

      @python-dev
      python-devmannequin
      on Jul 27, 2014
      Mannequin

      New changeset 5a2ec0a15017 by Martin v. Löwis in branch 'default':
      Issue bpo-20173: Convert sha1, sha256, sha512 and md5 to ArgumentClinic.
      http://hg.python.org/cpython/rev/5a2ec0a15017

    17. loewis commented on Jul 27, 2014

      @loewis
      loewismannequin
      on Jul 27, 2014
      Mannequin

      The codecsmodule still remains to be done.

    18. loewis commented on Jul 27, 2014

      @loewis
      loewismannequin
      on Jul 27, 2014
      Mannequin

      BTW, Vajrasky: Thanks for the patch!

    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 Apr 5, 2015

      @serhiy-storchaka
      serhiy-storchaka
      on Apr 5, 2015
      Member

      Here is reworked patch for the _codecs module. No optional groups are used, all parameters have sensible defaults. Default encoding is "utf-8", default errors is "strict" or None (if function accepts None), default mapping is None. Unified names and coding style, improved docstrings.

    22. serhiy-storchaka commented on Apr 16, 2015

      @serhiy-storchaka
      serhiy-storchaka
      on Apr 16, 2015
      Member

      The patch updated to the tip.

    23. python-dev commented on May 12, 2015

      @python-dev
      python-devmannequin
      on May 12, 2015
      Mannequin

      New changeset 031df83dffb4 by Serhiy Storchaka in branch 'default':
      Issue bpo-20173: Converted the _codecs module to Argument Clinic.
      https://hg.python.org/cpython/rev/031df83dffb4

    24. serhiy-storchaka commented on May 12, 2015

      @serhiy-storchaka
      serhiy-storchaka
      on May 12, 2015
      Member

      Updated to the tip and committed. Included Larry's suggestion about sys.getdefaultencoding().

    25. python-dev commented on May 12, 2015

      @python-dev
      python-devmannequin
      on May 12, 2015
      Mannequin

      New changeset 39df27d97901 by Serhiy Storchaka in branch 'default':
      Fixed compilation on Windows for issue bpo-20173.
      https://hg.python.org/cpython/rev/39df27d97901

    26. zware commented on Sep 22, 2016

      @zware
      zware
      on Sep 22, 2016
      Member

      This appears to be finished, please reopen if I'm mistaken.

    27. zware
      closed this as completedon Sep 22, 2016
    28. ezio-melotti
      transferred this issue fromon Apr 10, 2022
    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

      extension-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