classification
Title: Derby #15: Convert 50 sites to Argument Clinic across 9 files
Type: enhancement Stage: resolved
Components: Extension Modules Versions: Python 3.5
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: brett.cannon Nosy List: brett.cannon, larry, loewis, python-dev, serhiy.storchaka
Priority: normal Keywords: patch

Created on 2014-01-06 20:18 by brett.cannon, last changed 2014-11-10 01:26 by brett.cannon. This issue is now closed.

Files
File name Uploaded Description Edit
import_ac.diff brett.cannon, 2014-01-07 23:55 review
grp_derby.diff brett.cannon, 2014-01-17 19:05 grp module converted to Argument Clinic review
spwd_derby.diff brett.cannon, 2014-01-17 19:13 spwd converted to Argument Clinic review
pwd_derby.diff brett.cannon, 2014-01-17 19:29 pwd converted to Argument Clinic review
fcntl_derby.diff brett.cannon, 2014-01-21 15:19 fcntl converted to Argument Clinic (except for ioctl) review
pyexpat_derby.diff brett.cannon, 2014-01-31 13:49 pyexpat converted, relying on default self converter review
multibytecodec_derby.diff brett.cannon, 2014-01-31 13:56 multibytecodec with default self converter review
array_derby.diff brett.cannon, 2014-01-31 16:01 array module converted to AC review
cmath_derby.diff brett.cannon, 2014-01-31 20:20 all of cmath converted review
fcntl_derby.diff brett.cannon, 2014-11-07 17:44 review
Messages (35)
msg207474 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2014-01-06 20:18
Since _imp isn't directly exposed it isn't a priority to do the conversion, but for maintenance and such it should still occur.
msg207598 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-01-07 20:57
I'm normalizing the names of these issues so they're easier to read and to search for.
msg207625 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2014-01-07 23:55
I have attached my conversion of import.c. Larry, can you look it over to make sure I did it as expected? I tried to not have to write my own converter for O& as the docs seem to suggest object(converter='...') should work, but I got an error instead.
msg207641 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-01-08 00:21
(Brett: in order to avoid creating 129 differet issues on the tracker, I'm bundling files together into groups of 50 sites.  You don't have to tackle the whole list if you don't want to.)

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:
    Python/import.c: 8 sites
    Modules/cmathmodule.c: 8 sites
    Modules/cjkcodecs/multibytecodec.c: 8 sites
    Modules/arraymodule.c: 8 sites
    Modules/pyexpat.c: 7 sites
    Modules/fcntlmodule.c: 7 sites
    Modules/pwdmodule.c: 2 sites
    Modules/spwdmodule.c: 1 sites
    Modules/grpmodule.c: 1 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
msg207808 - (view) Author: Roundup Robot (python-dev) Date: 2014-01-10 00:03
New changeset 1f3242fb0c9c by Brett Cannon in branch 'default':
Issue #20152: import.c now uses Argument Clinic.
http://hg.python.org/cpython/rev/1f3242fb0c9c
msg207809 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2014-01-10 00:04
Now it's:

    Modules/cmathmodule.c: 8 sites
    Modules/cjkcodecs/multibytecodec.c: 8 sites
    Modules/arraymodule.c: 8 sites
    Modules/pyexpat.c: 7 sites
    Modules/fcntlmodule.c: 7 sites
    Modules/pwdmodule.c: 2 sites
    Modules/spwdmodule.c: 1 sites
    Modules/grpmodule.c: 1 sites
msg208347 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2014-01-17 17:09
I'm going to start tackling some of these today and checking them in directly. If I don't finish I will update the bug with what I didn't get to.
msg208351 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2014-01-17 18:38
Patch for multibytecodec.c.

Not sure how you calculated locations, Larry, but this file had 11 things to change, so some other work divisions might be off.
msg208353 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2014-01-17 18:55
After finding out that the initial site counts were off I went through and manually counted each file in this Derby group to get a more accurate idea of how much work each file would be.

    Modules/cmathmodule.c: 8 sites
    Modules/arraymodule.c: 27 sites
    Modules/pyexpat.c: 11 sites
    Modules/fcntlmodule.c: 4 sites
    Modules/pwdmodule.c: 3 sites
    Modules/spwdmodule.c: 2 sites
    Modules/grpmodule.c: 3 sites
msg208357 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2014-01-17 19:36
Might pick this up later today (or never), but taking a break for now.

Larry, I have patches for grp, spwd, _multibytecodec, and pwd I'd like you to give a once-over to make sure I didn't miss anything.

The only one that I think has an obvious improvement is in pwd where there is custom logic to deal with a PyArg_ParseTuple() failure. Does Argument Clinic have a way to specify custom logic in that instance?

With these patches the TODO for this derby group is:

    Modules/cmathmodule.c: 8 sites
    Modules/arraymodule.c: 27 sites
    Modules/pyexpat.c: 11 sites
    Modules/fcntlmodule.c: 4 sites

cmath might not be worth doing, though, for 3.4 as it already has custom macros to create function definitions.
msg208653 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2014-01-21 15:19
Here is fcntl converted. I didn't do fcntl.ioctl as it has a crazy set of possible signatures which I simply didn't want to deal with.
msg208668 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2014-01-21 16:36
Here is pyexpat converted. Couldn't do ErrorString as the 'l' format isn't supported; issue #20332.
msg209788 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2014-01-31 13:43
Here is pyexpat updated with ErrorString converted (and generally updated to manage changes now required by AC for class definitions).
msg209789 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2014-01-31 13:44
Should only have two modules left to convert:

    Modules/cmathmodule.c: 8 sites
    Modules/arraymodule.c: 27 sites

I have already started arraymodule.c; it's just taking a while.
msg209826 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2014-01-31 20:20
It took a bit of finessing but I managed to convert cmath in a way that didn't make it worse compared to before AC.

I now consider this part of the derby done and ready for Larry to review.
msg224141 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2014-07-27 16:10
multibytecodec_derby.diff looks fine, please apply.
msg224142 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2014-07-27 16:21
The cmath patch fails to apply; please update it.
msg225678 - (view) Author: Roundup Robot (python-dev) Date: 2014-08-22 15:45
New changeset c2d71f9afa0b by Brett Cannon in branch 'default':
Issue #20152: Convert _multibytecodecs to Argument Clinic.
http://hg.python.org/cpython/rev/c2d71f9afa0b
msg225679 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2014-08-22 15:47
To make it easier to track what is left to commit:

* grp
* spwd
* fcntl
* pyexpat
* array
* cmath [Martin says this no longer applies cleanly]

I should also mention all of these patches probably need to shift to `output preset file` as well.
msg225680 - (view) Author: Roundup Robot (python-dev) Date: 2014-08-22 15:52
New changeset 2617db7e43a3 by Brett Cannon in branch 'default':
Issue #20152: Convert the grp module to Argument Clinic.
http://hg.python.org/cpython/rev/2617db7e43a3
msg225681 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2014-08-22 15:53
To make it easier to track what is left to commit:
* spwd
* fcntl
* pyexpat
* array
* cmath [Martin says this no longer applies cleanly]
msg225691 - (view) Author: Roundup Robot (python-dev) Date: 2014-08-22 17:59
New changeset b3aa30f474c4 by Brett Cannon in branch 'default':
Issue #20152: Port the spwd module to Argument Clinic.
http://hg.python.org/cpython/rev/b3aa30f474c4
msg225692 - (view) Author: Roundup Robot (python-dev) Date: 2014-08-22 18:03
New changeset c48980af7df2 by Brett Cannon in branch 'default':
Issue #20152: Port the pwd module to Argument Clinic.
http://hg.python.org/cpython/rev/c48980af7df2
msg225694 - (view) Author: Roundup Robot (python-dev) Date: 2014-08-22 18:23
New changeset 0c2792a8f9db by Brett Cannon in branch 'default':
Issue #20152: Port pyexpat to Argument Clinic.
http://hg.python.org/cpython/rev/0c2792a8f9db
msg225695 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2014-08-22 18:24
To make it easier to track what is left to commit:
* fcntl [does not apply cleanly
* array
* cmath [Martin says this no longer applies cleanly]
msg229035 - (view) Author: Roundup Robot (python-dev) Date: 2014-10-10 20:26
New changeset f7ab0884467e by Brett Cannon in branch 'default':
Issue #20152: Port the array module to Argument Clinic.
https://hg.python.org/cpython/rev/f7ab0884467e
msg229036 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2014-10-10 20:27
Only ones left are:

- fcntl
- cmath

Both no longer apply cleanly.
msg229362 - (view) Author: Roundup Robot (python-dev) Date: 2014-10-14 21:37
New changeset 5e8b94397f81 by Brett Cannon in branch 'default':
Issue #20152: Convert the cmath module to Argument Clinic.
https://hg.python.org/cpython/rev/5e8b94397f81
msg230819 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2014-11-07 17:44
Here is a new patch for fcntl. I would like a review since I had to get a bit tricky to handle the polymorphic arguments for fcntl and ioctl and I don't think the test suite is that thorough for this module.
msg230822 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-11-07 19:32
I found bugs or doubtful code in the fcntl module. They should be fixed before converting to Argument Clinic.
msg230857 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2014-11-08 14:28
So I disagree that the code needs to be tweaked before converting to Argument Clinic. If the Clinic conversion is not adding to the problem then the code churn is just going to make applying this patch that much harder.

Thanks for the code review regardless, though!
msg230858 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-11-08 14:33
If first convert to Argument Clinic then fixing bugs will be much harder.
msg230859 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-11-08 14:43
About argument names. You have changed argument names and docstrings in any case (e.g. was "op", now "code"). Why not conform with standard documentation? This wouldn't add additional code churn if change it now. But will add if change it later.
msg230927 - (view) Author: Roundup Robot (python-dev) Date: 2014-11-10 01:23
New changeset 6e6532d313a1 by Brett Cannon in branch 'default':
Issue 20152, 22821: Port the fcntl module to Argument Clinic.
https://hg.python.org/cpython/rev/6e6532d313a1
msg230930 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2014-11-10 01:26
Finally finished!

I am never trusting Larry to count anything ever again. ;)
History
Date User Action Args
2014-11-10 01:26:34brett.cannonsetstatus: open -> closed
resolution: fixed
messages: + msg230930

stage: commit review -> resolved
2014-11-10 01:23:02python-devsetmessages: + msg230927
2014-11-08 14:43:09serhiy.storchakasetmessages: + msg230859
2014-11-08 14:33:24serhiy.storchakasetmessages: + msg230858
2014-11-08 14:28:37brett.cannonsetmessages: + msg230857
2014-11-07 19:32:50serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg230822
2014-11-07 17:44:20brett.cannonsetfiles: + fcntl_derby.diff

messages: + msg230819
2014-10-14 21:37:00python-devsetmessages: + msg229362
2014-10-10 20:27:10brett.cannonsetmessages: + msg229036
2014-10-10 20:26:40python-devsetmessages: + msg229035
2014-08-22 18:24:31brett.cannonsetmessages: + msg225695
2014-08-22 18:23:26python-devsetmessages: + msg225694
2014-08-22 18:03:58python-devsetmessages: + msg225692
2014-08-22 17:59:30python-devsetmessages: + msg225691
2014-08-22 15:53:19brett.cannonsetmessages: + msg225681
2014-08-22 15:52:51python-devsetmessages: + msg225680
2014-08-22 15:47:04brett.cannonsetmessages: + msg225679
stage: needs patch -> commit review
2014-08-22 15:45:10python-devsetmessages: + msg225678
2014-07-28 13:51:32brett.cannonsetassignee: larry -> brett.cannon
2014-07-27 16:21:09loewissetmessages: + msg224142
2014-07-27 16:10:01loewissetnosy: + loewis
messages: + msg224141
2014-01-31 20:20:57brett.cannonsetfiles: + cmath_derby.diff
assignee: brett.cannon -> larry
messages: + msg209826
2014-01-31 16:01:46brett.cannonsetfiles: + array_derby.diff
2014-01-31 13:56:20brett.cannonsetfiles: - multibytecodec_derby.diff
2014-01-31 13:56:06brett.cannonsetfiles: + multibytecodec_derby.diff
2014-01-31 13:49:12brett.cannonsetfiles: - pyexpat_derby.diff
2014-01-31 13:49:01brett.cannonsetfiles: + pyexpat_derby.diff
2014-01-31 13:45:02brett.cannonsetversions: + Python 3.5, - Python 3.4
2014-01-31 13:44:44brett.cannonsetassignee: larry -> brett.cannon
messages: + msg209789
2014-01-31 13:43:40brett.cannonsetfiles: + pyexpat_derby.diff

messages: + msg209788
2014-01-31 13:42:27brett.cannonsetfiles: - pyexpat_derby.diff
2014-01-21 16:36:23brett.cannonsetfiles: + pyexpat_derby.diff

messages: + msg208668
2014-01-21 15:19:54brett.cannonsetfiles: + fcntl_derby.diff

messages: + msg208653
2014-01-17 19:36:15brett.cannonsetassignee: brett.cannon -> larry
messages: + msg208357
2014-01-17 19:29:28brett.cannonsetfiles: + pwd_derby.diff
2014-01-17 19:21:18brett.cannonsetfiles: - multibytecodec_derby.diff
2014-01-17 19:21:00brett.cannonsetfiles: + multibytecodec_derby.diff
2014-01-17 19:13:08brett.cannonsetfiles: + spwd_derby.diff
2014-01-17 19:05:21brett.cannonsetfiles: + grp_derby.diff
2014-01-17 18:55:22brett.cannonsetmessages: + msg208353
2014-01-17 18:38:26brett.cannonsetfiles: + multibytecodec_derby.diff

messages: + msg208351
2014-01-17 17:09:16brett.cannonsetassignee: brett.cannon
messages: + msg208347
2014-01-10 00:04:25brett.cannonsetassignee: larry -> (no value)
2014-01-10 00:04:10brett.cannonsetmessages: + msg207809
2014-01-10 00:03:35python-devsetnosy: + python-dev
messages: + msg207808
2014-01-08 01:36:37r.david.murraylinkissue20187 dependencies
2014-01-08 00:21:06larrysetstatus: pending -> open
priority: low -> normal
title: Derby: Convert the _imp module to use Argument Clinic -> Derby #15: Convert 50 sites to Argument Clinic across 9 files
messages: + msg207641

type: enhancement
2014-01-07 23:55:50brett.cannonsetstatus: open -> pending
files: + import_ac.diff
messages: + msg207625

assignee: brett.cannon -> larry
keywords: + patch
2014-01-07 20:57:10larrysetmessages: + msg207598
title: Use Argument Clinic with _imp -> Derby: Convert the _imp module to use Argument Clinic
2014-01-06 20:18:22brett.cannonsetcomponents: + Extension Modules
2014-01-06 20:18:13brett.cannoncreate