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

[larryhastings]

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]

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

[vajrasky]

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]

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]

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

[larryhastings]

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]

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]

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]

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]

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]

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]

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