Repository navigation
Derby #11: Convert 50 sites to Argument Clinic across 9 files #64379
Description
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 2018-09-12.18:56:33.809>
created_at = <Date 2014-01-08.00:16:02.924>
labels = ['extension-modules', 'type-feature', '3.8', 'expert-argument-clinic']
title = 'Derby #11: Convert 50 sites to Argument Clinic across 9 files'
updated_at = <Date 2019-04-12.07:11:33.603>
user = 'https://github.com/larryhastings'bugs.python.org fields:
activity = <Date 2019-04-12.07:11:33.603>
actor = 'methane'
assignee = 'none'
closed = True
closed_date = <Date 2018-09-12.18:56:33.809>
closer = 'rhettinger'
components = ['Extension Modules', 'Argument Clinic']
creation = <Date 2014-01-08.00:16:02.924>
creator = 'larry'
dependencies = []
files = ['33535', '33578', '33579', '33604', '33620', '33621', '33625', '33627', '33749', '39169', '39190', '39191', '46292', '46297']
hgrepos = []
issue_num = 20180
keywords = ['patch']
message_count = 63.0
messages = ['207636', '208431', '208587', '208589', '208728', '208803', '208804', '208821', '208827', '208830', '208846', '208855', '208861', '208865', '208873', '209450', '209451', '224137', '224761', '241488', '241819', '241923', '241925', '241927', '241930', '241931', '241934', '242482', '247955', '247987', '248513', '248663', '285498', '285500', '285514', '285516', '285554', '285558', '285559', '285561', '285562', '285563', '286435', '304904', '304931', '304935', '304936', '304937', '304972', '304976', '304981', '305409', '319533', '319933', '320830', '321161', '321166', '324948', '325072', '325121', '325167', '332505', '340011']
nosy_count = 10.0
nosy_names = ['loewis', 'rhettinger', 'taleinat', 'larry', 'rbcollins', 'methane', 'skrah', 'python-dev', 'martin.panter', 'serhiy.storchaka']
pr_nums = ['4117', '4170', '8039', '9164', '9828', '12792']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue20180'
versions = ['Python 3.8']
Activity
larryhastings commentedon Jan 8, 2014
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/_collectionsmodule.c: 4 sites
Modules/itertoolsmodule.c: 11 sites
Modules/_randommodule.c: 1 sites
Objects/unicodeobject.c: 14 sites
Modules/xxlimited.c: 4 sites
Modules/xxmodule.c: 5 sites
Modules/xxsubtype.c: 5 sites
Objects/stringlib/find.h: 1 sites
Objects/stringlib/transmogrify.h: 5 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
taleinat commentedon Jan 19, 2014
I'm attaching my conversion of deque.rotate and _count_elements in Modules/_collectionsmodule.c.
There remain 3 other functions in that module to be converted.
One is deque_init which returns int; Larry said (on IRC) to hold off with that for now.
The other two are dequeiter_new and dequereviter_new, which use the O! format, which I am unsure how to handle.
taleinat commentedon Jan 20, 2014
Attaching patch for complete conversion of Objects/unicodeobject.c.
Notes:
taleinat commentedon Jan 20, 2014
Attached patch for AC conversion of Objects/stringlib/transmogrify.h.
Converting the functions in this file required changes to Objects/bytesobject.c and Objects/bytearrayobject.c. Those changes are in the patch as well. The conversion of those files is part of bpo-20179 (Derby #10). I'm adding a comment on that issue as well with a reference to this.
taleinat commentedon Jan 22, 2014
Attaching a new patch for complete conversion of Objects/unicodeobject.c.
This is to replace the previous patch for this file.
This new patch also converts all other possible unicode methods,
including those not using PyArg_* functions. This includes various
methods with no arguments or with one PyObject* argument.
Unicode methods still not converted, probably impossible to convert:
because they use a special function
'stringlib_parse_args_finds_unicode' to parse their arguments
taleinat commentedon Jan 22, 2014
Attached nearly complete conversion of Modules/_collectionsmodule.c.
I've converted all functions using PyArg_* functions, including __new__ methods, but not deque.__init__.
I'm attaching a separate patch for deque.__init__.
taleinat commentedon Jan 22, 2014
Attached a patch converting deque.__init__ in Modules/_collectionsmodule.c, which completes the conversion of this file.
This patch depends Larry's first patch in bpo-20341 (Argument Clinic: add "nullable ints"). This functionality is used to simplify the existing code for accepting None for the 'maxlen' argument to deque.__init__. If needed I can supply a patch which does not depend on Larry's patch.
taleinat commentedon Jan 22, 2014
Attached is a complete conversion of Modules/_randommodule.c. Three _random.Random methods were converted: seed, getrandbits and __new__.
There's was an problem converting _random.Random.__new__. I overcame it cleanly and think this patch is good. Details about the problem follow.
bpo-1486663 made various __new__ methods of built-in classes only do a '_PyArg_NoKeywords' check if the type is the class itself, i.e. not for sub-classes. This is to allow sub-classes to accept keyword arguments.
This means that for classes that do this, like _random.Random, converting using AC makes it so subclasses can't accept keyword arguments (unless the subclasses implement __new__ properly).
Still, it is possible to convert _random.Random.__new__ and also override __new__ in random.Random (random != _random, and random.Random subclasses _random.Random). This way subclasses of the latter can accept keyword arguments. Since anyone sub-classing _random.Random instead of random.Random can be expected to know to override __new__ or switch to random.Random, I think this is a good solution for this case.
The attached patch does as I suggest above and passes all tests in test_random, including 'test_random_subclass_with_kwargs' which checks this issue. Just to be sure, I also ran the entire test suite, and all tests pass.
taleinat commentedon Jan 22, 2014
Attached complete conversion of Modules/xxlimited.c, Modules/xxmodule.c and Modules/xxsubtype.c.
There are no tests for these and the code is not used by anything AFAIK, so I was extra careful and went over the diffs manually once everything was done.
skrah commentedon Jan 22, 2014
I think we should probably not convert xx*.c. They are for newcomers
who will likely be confused by the additional information overload.
taleinat commentedon Jan 22, 2014
I thought AC was how all new modules were supposed to be written. If so, the example modules should definitely use AC!
Also note that once the generated code is in separate files, the code in the origin files will be simpler and clearer.
skrah commentedon Jan 22, 2014
The vast majority of C modules are in third-party packages. I doubt that external
modules will all use AC. xxmodule.c is about explaining the basic structure of
a C extension. AC adds nothing to this and obfuscates (IMO) what is going on.
taleinat commentedon Jan 22, 2014
Thanks, Stefan. I hadn't realized that the xx files were targeted mainly a third-party developers. In this case, I can see the sense in not converting them.
I only wish someone had said something *before* I went ahead and converted them...
41 remaining items
taleinat commentedon Jun 14, 2018
IMO we should create new issues for AC conversion of the collections and random modules (assuming they haven't been converted yet), and close this issue.
methane commentedon Jun 19, 2018
I agree with you. Let's finish derby.
taleinat commentedon Jul 1, 2018
See updated PR for Objects/stringlib/transmogrify.h in #52287.
taleinat commentedon Jul 6, 2018
New changeset c929df3 by Tal Einat in branch 'master':
bpo-20180: complete AC conversion of Objects/stringlib/transmogrify.h (GH-8039)
c929df3
taleinat commentedon Jul 6, 2018
Now that transmogrify is done, the only open question is about itertools and collections.
rhettinger commentedon Sep 10, 2018
New changeset 3286ce4 by Raymond Hettinger (Tal Einat) in branch 'master':
bpo-20180: itertools.groupby Argument Clinic conversion (GH-4170)
3286ce4
rhettinger commentedon Sep 11, 2018
New changeset c4bccd3 by Raymond Hettinger (Tal Einat) in branch 'master':
bpo-20180: convert most of itertoolsmodule.c to use Argument Clinic (GH-9164)
c4bccd3
taleinat commentedon Sep 12, 2018
Now just the collections module is left from this group.
Raymond, what would you like done with it WRT AC conversion, if at all?
rhettinger commentedon Sep 12, 2018
I would like to pass on the collections module. Having looked at the patch, I think the code is currently better as is.
That bring this one Derby to a close. Congratulations :-)
serhiy-storchaka commentedon Dec 25, 2018
New changeset 65ce60a by Serhiy Storchaka in branch 'master':
bpo-20180: Simplify char_converter in Argument Clinic. (GH-9828)
65ce60a
methane commentedon Apr 12, 2019
New changeset 9e4f2f3 by Inada Naoki in branch 'master':
bpo-20180: Use argument clinic for dict.pop() and dict.popitem() (GH-12792)
9e4f2f3