gh-104469 : Convert _testcapi/dict.c to use AC

[jamie2779]

• Issue: Improve the readability and maintainability of test_capi using the AC #104469

[bedevere-bot]

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

[jamie2779]

@corona10

[corona10]

It looks like str(accept={str, NoneType}, zeroes=True) according to https://docs.python.org/3/howto/clinic.html?highlight=clinic#how-to-use-real-argument-clinic-converters-instead-of-legacy-converters

[corona10]

Hi @erlend-aasland and @AlexWaygood it doesn't work when we use str(accept={str, NoneType}, zeroes=True) grammar even if documentation support it. Would you like to take a look?

Error in file 'Modules/_testcapi/dict.c' on line 260:
str_converter: illegal combination of arguments (frozenset({<class 'NoneType'>, <class 'str'>}), False, True)

[erlend-aasland]

The docs are inaccurate; the accept parameter should be accept={robuffer, str, NoneType}. See the definition in clinic.py:

cpython/Tools/clinic/clinic.py

Line 3927 in 23a6db9

r('z#', zeroes=True, accept={robuffer, str, NoneType})

[erlend-aasland]

Also, it is possible to just use the legacy converter:

Suggested change

	attr_name: str(accept={robuffer}, zeroes=True)
	attr_name: 'z#'

[erlend-aasland]

This is not correct; dict_getitemref only takes two positional-only arguments: obj and attr_name.

[erlend-aasland]

Ditto. value is not part of the spec of this method; dict_getitemstringref only takes two positional-only arguments.

[erlend-aasland]

The order is incorrect. It should be mapping, key, value.

[erlend-aasland]

The docs are inaccurate; the accept parameter should be accept={robuffer, str, NoneType}. See the definition in clinic.py:

cpython/Tools/clinic/clinic.py

Line 3927 in 23a6db9

r('z#', zeroes=True, accept={robuffer, str, NoneType})

[erlend-aasland]

Also, it is possible to just use the legacy converter:

Suggested change

	attr_name: str(accept={robuffer}, zeroes=True)
	attr_name: 'z#'

[bedevere-bot]

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

[serhiy-storchaka]

It significantly reduces readability to me.

It significantly increases the number of lines in Modules/_testcapi/dict.c:

 Modules/_testcapi/dict.c | 412 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++----------------------------------------------
 1 file changed, 291 insertions(+), 121 deletions(-)

And this is not counting the large amount of code added to another file.

I have a limited field of vision and can't see anything beyond a limited number of lines of code. Before that, test functions were compact. After these changes, they will exceed my limit. And if I need to understand what exactly the function does, I have to look for the corresponding code in the generated file and then go back to the previous file, but when I look from the file I lose the context and I have to reread each line on the screen again to find the right place.

These changes are not friendly to the visually impaired.

[jamie2779]

@erlend-aasland @corona10 I have made the requested changes; Please Take Another Look!

[erlend-aasland]

I think Serhiy's opinion should be taken into account. Converting _testcapi to Argument Clinic is not of importance.

[ambv]

Closing and re-opening to retrigger CLA checks. Sorry for the noise.

[cpython-cla-bot]

The following commit authors need to sign the Contributor License Agreement:

• [email protected]

Click the button to sign:

[corona10]

close via #104469 (comment)