FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

gh-104469 : Convert _testcapi/dict.c to use AC by jamie2779 · Pull Request #107859 · python/cpython · GitHub

  1. python
  2. cpython

Repository navigation

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

Closed
jamie2779 wants to merge 2 commits into
python:mainfrom
jamie2779:gh-104469
ConversationCommits (2)ChecksFiles changed
Closed

gh-104469 : Convert _testcapi/dict.c to use AC#107859
jamie2779 wants to merge 2 commits into
python:mainfrom
jamie2779:gh-104469

Conversation

jamie2779 commented Aug 11, 2023 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Contributor

bedevere-bot mentioned this pull request Aug 11, 2023
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.

jamie2779 commented Aug 11, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@corona10

corona10 self-assigned this 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

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 Aug 11, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 Aug 11, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 Aug 11, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

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

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

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

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

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

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

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

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

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

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

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

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

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.

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.

jamie2779 commented Aug 11, 2023

Copy link
Copy Markdown
Contributor Author

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

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.

ambv commented Aug 11, 2023

Copy link
Copy Markdown
Contributor

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

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:

Click the button to sign:

corona10 commented Aug 11, 2023

Copy link
Copy Markdown
Member

close via #104469 (comment)

corona10 closed this Aug 11, 2023
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

corona10 corona10 left review comments
erlend-aasland Awaiting requested review from erlend-aasland

Assignees

corona10

Labels

awaiting changes skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

Back | FazBrowse Home | New Git URL