classification
Title: Derby #4: Convert 53 sites to Argument Clinic across 5 files
Type: enhancement Stage: resolved
Components: Argument Clinic, Extension Modules Versions: Python 3.5
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: christian.heimes, larry, loewis, python-dev, serhiy.storchaka, vajrasky, zach.ware
Priority: normal Keywords: patch

Created on 2014-01-07 23:46 by larry, last changed 2016-09-22 05:07 by zach.ware. This issue is now closed.

Files
File name Uploaded Description Edit
clinic_sha1module.patch vajrasky, 2014-01-08 11:20 review
clinic_sha1module_v2.patch vajrasky, 2014-01-09 08:28 review
clinic_sha1module_v3.patch vajrasky, 2014-01-10 04:06 review
clinic_sha256module.patch vajrasky, 2014-01-10 07:29 review
clinic_sha512module.patch vajrasky, 2014-01-10 08:48 review
clinic_md5module.patch vajrasky, 2014-01-10 09:29 review
clinic_codecsmodule.patch vajrasky, 2014-01-13 15:28 review
clinic_sha1module_v4.patch vajrasky, 2014-01-23 09:03 review
clinic_sha256module_v2.patch vajrasky, 2014-01-23 09:24 review
clinic_sha512module_v2.patch vajrasky, 2014-01-23 09:46 review
clinic_md5module_v2.patch vajrasky, 2014-01-23 10:38 review
clinic_codecsmodule_v2.patch vajrasky, 2014-01-24 08:43 review
clinic_sha1module_v5.patch vajrasky, 2014-01-27 04:12 review
clinic_sha256module_v3.patch vajrasky, 2014-01-27 06:41 review
clinic_sha512module_v3.patch vajrasky, 2014-01-27 08:17 review
clinic_md5module_v3.patch vajrasky, 2014-01-27 08:47 review
issue20173_conglomerate.patch vajrasky, 2014-02-04 09:12 review
codecs_clinic.patch serhiy.storchaka, 2015-04-05 13:53 review
codecs_clinic_2.patch serhiy.storchaka, 2015-04-16 15:54 review
Messages (38)
msg207618 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-01-07 23:46
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
msg207679 - (view) Author: Vajrasky Kok (vajrasky) * Date: 2014-01-08 11:20
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.
msg207680 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-01-08 11:52
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.
msg207690 - (view) Author: Vajrasky Kok (vajrasky) * Date: 2014-01-08 16:09
An example how to convert keyword argument would be very helpful. I searched the example from existing code but nothing shows up.
msg207691 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-01-08 16:16
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.
msg207732 - (view) Author: Vajrasky Kok (vajrasky) * Date: 2014-01-09 08:28
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.
msg207823 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-01-10 02:21
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
msg207828 - (view) Author: Vajrasky Kok (vajrasky) * Date: 2014-01-10 03:12
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.... >.<
msg207829 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-01-10 03:14
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.
msg207831 - (view) Author: Vajrasky Kok (vajrasky) * Date: 2014-01-10 03:35
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.");
msg207833 - (view) Author: Vajrasky Kok (vajrasky) * Date: 2014-01-10 04:06
>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).
msg207836 - (view) Author: Vajrasky Kok (vajrasky) * Date: 2014-01-10 07:29
Here is the patch for sha256. It got the same issue as sha1. After being clinic-ed, the constructor accepts None value happily.
msg207837 - (view) Author: Vajrasky Kok (vajrasky) * Date: 2014-01-10 08:48
Here is the patch for sha512. It got the same issue as sha1. After being clinic-ed, the constructor accepts None value happily.
msg207838 - (view) Author: Vajrasky Kok (vajrasky) * Date: 2014-01-10 09:29
Here is the patch for md5. It got the same issue as sha1. After being clinic-ed, the constructor accepts None value happily.
msg208021 - (view) Author: Vajrasky Kok (vajrasky) * Date: 2014-01-13 11:29
Here is the patch for _codecs module.
msg208248 - (view) Author: Vajrasky Kok (vajrasky) * Date: 2014-01-16 07:53
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.
msg208249 - (view) Author: Vajrasky Kok (vajrasky) * Date: 2014-01-16 07:53
s/Guide/Guido.
msg208888 - (view) Author: Vajrasky Kok (vajrasky) * Date: 2014-01-23 09:03
Here is the updated patch of sha1module, based on Larry's review. Thanks!
msg208895 - (view) Author: Vajrasky Kok (vajrasky) * Date: 2014-01-23 09:24
Here is the updated patch of sha256module, based on Larry's review. Thanks!
msg208902 - (view) Author: Vajrasky Kok (vajrasky) * Date: 2014-01-23 09:46
Here is the updated patch of sha512module, based on Larry's review. Thanks!
msg208907 - (view) Author: Vajrasky Kok (vajrasky) * Date: 2014-01-23 10:38
Here is the updated patch of md5module, based on Larry's review. Thanks!
msg209046 - (view) Author: Vajrasky Kok (vajrasky) * Date: 2014-01-24 08:43
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.
msg209392 - (view) Author: Vajrasky Kok (vajrasky) * Date: 2014-01-27 04:12
Here is the updated patch for sha1module after changes from Larry to clinic.
msg209403 - (view) Author: Vajrasky Kok (vajrasky) * Date: 2014-01-27 06:41
Here is the updated patch for sha256module after changes from Larry to clinic.
msg209415 - (view) Author: Vajrasky Kok (vajrasky) * Date: 2014-01-27 08:17
Here is the updated patch for sha512module after changes from Larry to clinic.
msg209417 - (view) Author: Vajrasky Kok (vajrasky) * Date: 2014-01-27 08:47
Here is the updated patch for md5module after changes from Larry to clinic.
msg209420 - (view) Author: Vajrasky Kok (vajrasky) * Date: 2014-01-27 09:14
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
msg210185 - (view) Author: Vajrasky Kok (vajrasky) * Date: 2014-02-04 09:12
Here is the updated patch after Larry's commit to clinic. Everything is included except codecsmodule.
msg224122 - (view) Author: Roundup Robot (python-dev) Date: 2014-07-27 12:20
New changeset 5a2ec0a15017 by Martin v. Löwis in branch 'default':
Issue #20173: Convert sha1, sha256, sha512 and md5 to ArgumentClinic.
http://hg.python.org/cpython/rev/5a2ec0a15017
msg224123 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2014-07-27 12:21
The codecsmodule still remains to be done.
msg224124 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2014-07-27 12:22
BTW, Vajrasky: Thanks for the patch!
msg224755 - (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.
msg240120 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-04-05 13:53
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.
msg241227 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-04-16 15:54
The patch updated to the tip.
msg242952 - (view) Author: Roundup Robot (python-dev) Date: 2015-05-12 10:18
New changeset 031df83dffb4 by Serhiy Storchaka in branch 'default':
Issue #20173: Converted the _codecs module to Argument Clinic.
https://hg.python.org/cpython/rev/031df83dffb4
msg242953 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-05-12 10:18
Updated to the tip and committed. Included Larry's suggestion about sys.getdefaultencoding().
msg242959 - (view) Author: Roundup Robot (python-dev) Date: 2015-05-12 11:01
New changeset 39df27d97901 by Serhiy Storchaka in branch 'default':
Fixed compilation on Windows for issue #20173.
https://hg.python.org/cpython/rev/39df27d97901
msg277189 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2016-09-22 05:07
This appears to be finished, please reopen if I'm mistaken.
History
Date User Action Args
2016-09-22 05:07:15zach.waresetstatus: open -> closed

nosy: + zach.ware
messages: + msg277189

resolution: fixed
stage: patch review -> resolved
2015-05-12 11:01:02python-devsetmessages: + msg242959
2015-05-12 10:18:59serhiy.storchakasetmessages: + msg242953
2015-05-12 10:18:08python-devsetmessages: + msg242952
2015-04-16 15:54:29serhiy.storchakasetfiles: + codecs_clinic_2.patch

messages: + msg241227
2015-04-05 13:53:20serhiy.storchakasetfiles: + codecs_clinic.patch

nosy: + serhiy.storchaka
messages: + msg240120

stage: needs patch -> patch review
2015-02-25 15:27:09serhiy.storchakasetcomponents: + Argument Clinic
2014-08-04 20:12:23larrysetmessages: + msg224755
versions: + Python 3.5, - Python 3.4
2014-07-27 12:22:16loewissetmessages: + msg224124
2014-07-27 12:21:41loewissetnosy: + loewis
messages: + msg224123
2014-07-27 12:20:45python-devsetnosy: + python-dev
messages: + msg224122
2014-02-04 09:12:40vajraskysetfiles: + issue20173_conglomerate.patch

messages: + msg210185
2014-01-27 09:14:09vajraskysetmessages: + msg209420
2014-01-27 08:47:05vajraskysetfiles: + clinic_md5module_v3.patch

messages: + msg209417
2014-01-27 08:17:13vajraskysetfiles: + clinic_sha512module_v3.patch

messages: + msg209415
2014-01-27 06:41:18vajraskysetfiles: + clinic_sha256module_v3.patch

messages: + msg209403
2014-01-27 04:12:29vajraskysetfiles: + clinic_sha1module_v5.patch

messages: + msg209392
2014-01-24 08:43:50vajraskysetfiles: + clinic_codecsmodule_v2.patch

messages: + msg209046
2014-01-23 10:38:36vajraskysetfiles: + clinic_md5module_v2.patch

messages: + msg208907
2014-01-23 09:46:29vajraskysetfiles: + clinic_sha512module_v2.patch

messages: + msg208902
2014-01-23 09:24:07vajraskysetfiles: + clinic_sha256module_v2.patch

messages: + msg208895
2014-01-23 09:03:49vajraskysetfiles: + clinic_sha1module_v4.patch

messages: + msg208888
2014-01-16 07:53:19vajraskysetmessages: + msg208249
2014-01-16 07:53:03vajraskysetmessages: + msg208248
2014-01-13 15:28:23vajraskysetfiles: + clinic_codecsmodule.patch
2014-01-13 15:18:36vajraskysetfiles: - clinic_codecsmodule.patch
2014-01-13 11:29:36vajraskysetfiles: + clinic_codecsmodule.patch

messages: + msg208021
2014-01-10 09:29:47vajraskysetfiles: + clinic_md5module.patch

messages: + msg207838
2014-01-10 08:48:25vajraskysetfiles: + clinic_sha512module.patch

messages: + msg207837
2014-01-10 07:29:10vajraskysetfiles: + clinic_sha256module.patch

messages: + msg207836
2014-01-10 04:06:55vajraskysetfiles: + clinic_sha1module_v3.patch

messages: + msg207833
2014-01-10 03:35:31vajraskysetmessages: + msg207831
2014-01-10 03:14:12larrysetmessages: + msg207829
2014-01-10 03:12:27vajraskysetnosy: + christian.heimes
messages: + msg207828
2014-01-10 02:21:42larrysetmessages: + msg207823
2014-01-09 08:28:05vajraskysetfiles: + clinic_sha1module_v2.patch

messages: + msg207732
2014-01-08 16:16:01larrysetmessages: + msg207691
2014-01-08 16:09:00vajraskysetmessages: + msg207690
2014-01-08 11:52:39larrysetmessages: + msg207680
2014-01-08 11:20:44vajraskysetfiles: + clinic_sha1module.patch

nosy: + vajrasky
messages: + msg207679

keywords: + patch
2014-01-08 01:36:37r.david.murraylinkissue20187 dependencies
2014-01-07 23:52:36larrysettype: behavior -> enhancement
2014-01-07 23:46:33larrycreate