GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — gh-104469 : Convert _testcapi/dict.c to use AC by jamie2779 · Pull Request #107859 · python/cpython

Conversation

jamie2779 commented Aug 11, 2023 •

edited by bedevere-bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          Convert_testcapi/dict.c to use AC

8dc589e

bedevere-bot added the awaiting review label

Aug 11, 2023

bedevere-bot mentioned this pull request

Aug 11, 2023

Improve the readability and maintainability of test_capi using the AC #104469

Closed

bedevere-bot commented Aug 11, 2023

Copy link

Copy Markdown

Most changes to Python require a NEWS entry.

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

jamie2779 commented Aug 11, 2023 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor Author

@corona10

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

corona10 self-assigned this

Aug 11, 2023

corona10 added the skip news label

Aug 11, 2023

corona10 reviewed

Aug 11, 2023

View reviewed changes

Comment thread

Modules/_testcapi/dict.c Outdated

		_testcapi.dict_getitemstringref
		obj: object
		value: object
		attr_name: str(accept={robuffer}, zeroes=True)

corona10 Aug 11, 2023

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

corona10 Aug 11, 2023

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

erlend-aasland Aug 11, 2023

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

erlend-aasland Aug 11, 2023

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Suggested change

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

corona10 requested a review from erlend-aasland

August 11, 2023 09:03

erlend-aasland requested changes

Aug 11, 2023

View reviewed changes

Comment thread

Modules/_testcapi/dict.c Outdated

		_testcapi.dict_getitemref
		obj: object
		attr_name: object
		value: object

erlend-aasland Aug 11, 2023

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Modules/_testcapi/dict.c Outdated

		/*[clinic input]
		_testcapi.dict_getitemstringref
		obj: object
		value: object

erlend-aasland Aug 11, 2023

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Modules/_testcapi/dict.c Outdated

Comment on lines +311 to +312

		value: object
		key: str(accept={robuffer}, zeroes=True)

erlend-aasland Aug 11, 2023

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Modules/_testcapi/dict.c Outdated

		_testcapi.dict_getitemstringref
		obj: object
		value: object
		attr_name: str(accept={robuffer}, zeroes=True)

erlend-aasland Aug 11, 2023

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Modules/_testcapi/dict.c Outdated

		_testcapi.dict_getitemstringref
		obj: object
		value: object
		attr_name: str(accept={robuffer}, zeroes=True)

erlend-aasland Aug 11, 2023

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Suggested change

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

bedevere-bot removed the awaiting review label

Aug 11, 2023

bedevere-bot commented Aug 11, 2023

Copy link

Copy Markdown

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bedevere-bot added the awaiting changes label

Aug 11, 2023

serhiy-storchaka commented Aug 11, 2023

Copy link

Copy Markdown

Member

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.

erlend-aasland reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


          Fix clinic input

11af5f9

jamie2779 requested a review from erlend-aasland

August 11, 2023 13:16

jamie2779 commented Aug 11, 2023

Copy link

Copy Markdown

Contributor Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

erlend-aasland commented Aug 11, 2023

Copy link

Copy Markdown

Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ambv commented Aug 11, 2023

Copy link

Copy Markdown

Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ambv closed this

Aug 11, 2023

ambv reopened this

Aug 11, 2023

ghost commented Aug 11, 2023

Copy link

Copy Markdown

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

[email protected]

Click the button to sign:

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

corona10 commented Aug 11, 2023

Copy link

Copy Markdown

Member

close via #104469 (comment)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

corona10 closed this

Aug 11, 2023

Reviewers

corona10

corona10 left review comments

erlend-aasland

Awaiting requested review from erlend-aasland

Uh oh!

There was an error while loading. Please reload this page.

Assignees

corona10

Labels

awaiting changes skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

Conversation

jamie2779 commented Aug 11, 2023 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bedevere-bot commented Aug 11, 2023

Uh oh!

jamie2779 commented Aug 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Aug 11, 2023

Uh oh!

serhiy-storchaka commented Aug 11, 2023

Uh oh!

jamie2779 commented Aug 11, 2023

Uh oh!

erlend-aasland commented Aug 11, 2023

Uh oh!

ambv commented Aug 11, 2023

Uh oh!

ghost commented Aug 11, 2023

Uh oh!

corona10 commented Aug 11, 2023

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

jamie2779 commented Aug 11, 2023 •

edited by bedevere-bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

jamie2779 commented Aug 11, 2023 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.