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 #4: Convert 53 sites to Argument Clinic across 5 files #64372

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

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

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

Comments

@larryhastings
Copy link
Contributor

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']

    @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/_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

    @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
    @vajrasky
    Copy link
    Mannequin

    vajrasky mannequin commented Jan 8, 2014

    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.

    @larryhastings
    Copy link
    Contributor Author

    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.

    @vajrasky
    Copy link
    Mannequin

    vajrasky mannequin commented Jan 8, 2014

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

    @larryhastings
    Copy link
    Contributor Author

    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.

    @vajrasky
    Copy link
    Mannequin

    vajrasky mannequin commented Jan 9, 2014

    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.

    @larryhastings
    Copy link
    Contributor Author

    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

    @vajrasky
    Copy link
    Mannequin

    vajrasky mannequin commented Jan 10, 2014

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

    @larryhastings
    Copy link
    Contributor Author

    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.

    @vajrasky
    Copy link
    Mannequin

    vajrasky mannequin commented Jan 10, 2014

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

    @vajrasky
    Copy link
    Mannequin

    vajrasky mannequin commented Jan 10, 2014

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

    @vajrasky
    Copy link
    Mannequin

    vajrasky mannequin commented Jan 10, 2014

    Here is the patch for sha256. It got the same issue as sha1. After being clinic-ed, the constructor accepts None value happily.

    @vajrasky
    Copy link
    Mannequin

    vajrasky mannequin commented Jan 10, 2014

    Here is the patch for sha512. It got the same issue as sha1. After being clinic-ed, the constructor accepts None value happily.

    @vajrasky
    Copy link
    Mannequin

    vajrasky mannequin commented Jan 10, 2014

    Here is the patch for md5. It got the same issue as sha1. After being clinic-ed, the constructor accepts None value happily.

    @vajrasky
    Copy link
    Mannequin

    vajrasky mannequin commented Jan 13, 2014

    Here is the patch for _codecs module.

    @vajrasky
    Copy link
    Mannequin

    vajrasky mannequin commented Jan 16, 2014

    So, as Guide said in https://mail.python.org/pipermail/python-dev/2014-January/131675.html:

    "In the sha1 example, however, accepting None and converting it to NULL
    (without a reference leak, please :-) seems fine though."

    I let the patches as they are.

    @vajrasky
    Copy link
    Mannequin

    vajrasky mannequin commented Jan 16, 2014

    s/Guide/Guido.

    @vajrasky
    Copy link
    Mannequin

    vajrasky mannequin commented Jan 23, 2014

    Here is the updated patch of sha1module, based on Larry's review. Thanks!

    @vajrasky
    Copy link
    Mannequin

    vajrasky mannequin commented Jan 23, 2014

    Here is the updated patch of sha256module, based on Larry's review. Thanks!

    @vajrasky
    Copy link
    Mannequin

    vajrasky mannequin commented Jan 23, 2014

    Here is the updated patch of sha512module, based on Larry's review. Thanks!

    @vajrasky
    Copy link
    Mannequin

    vajrasky mannequin commented Jan 23, 2014

    Here is the updated patch of md5module, based on Larry's review. Thanks!

    @vajrasky
    Copy link
    Mannequin

    vajrasky mannequin commented Jan 24, 2014

    Here is the updated patch of codecsmodule, based on Larry's review. Thanks! I didn't realize clinic releases pybuffer implicitly. I also turned _codecs.Codecs to _codecs since all of these methods are module methods.

    @vajrasky
    Copy link
    Mannequin

    vajrasky mannequin commented Jan 27, 2014

    Here is the updated patch for sha1module after changes from Larry to clinic.

    @vajrasky
    Copy link
    Mannequin

    vajrasky mannequin commented Jan 27, 2014

    Here is the updated patch for sha256module after changes from Larry to clinic.

    @vajrasky
    Copy link
    Mannequin

    vajrasky mannequin commented Jan 27, 2014

    Here is the updated patch for sha512module after changes from Larry to clinic.

    @vajrasky
    Copy link
    Mannequin

    vajrasky mannequin commented Jan 27, 2014

    Here is the updated patch for md5module after changes from Larry to clinic.

    @vajrasky
    Copy link
    Mannequin

    vajrasky mannequin commented Jan 27, 2014

    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

    @vajrasky
    Copy link
    Mannequin

    vajrasky mannequin commented Feb 4, 2014

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

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jul 27, 2014

    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

    @loewis
    Copy link
    Mannequin

    loewis mannequin commented Jul 27, 2014

    The codecsmodule still remains to be done.

    @loewis
    Copy link
    Mannequin

    loewis mannequin commented Jul 27, 2014

    BTW, Vajrasky: Thanks for the patch!

    @larryhastings
    Copy link
    Contributor Author

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

    @serhiy-storchaka
    Copy link
    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.

    @serhiy-storchaka
    Copy link
    Member

    The patch updated to the tip.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented May 12, 2015

    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

    @serhiy-storchaka
    Copy link
    Member

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

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented May 12, 2015

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

    @zware
    Copy link
    Member

    zware commented Sep 22, 2016

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

    @zware zware closed this as completed Sep 22, 2016
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    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

    3 participants