GV

GeezerVison

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

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-104683: Argument Clinic: Extract parse_function_names() helper by erlend-aasland · Pull Request #107964 · python/cpython

Conversation

@erlend-aasland

@erlend-aasland erlend-aasland commented Aug 15, 2023 •
edited by bedevere-bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

Reduce duplicate code in state_modulename_name()

  • Issue: Modernise code in Tools/clinic/ #104683

Sorry, something went wrong.

Uh oh!

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

All reactions
@erlend-aasland
pythongh-104683: Argument Clinic: Extract parse_names() helper
2f53112
Reduce duplicate code in state_modulename_name()
@erlend-aasland
erlend-aasland requested a review from AlexWaygood as a code owner August 15, 2023 08:54
@erlend-aasland erlend-aasland added the skip news label Aug 15, 2023
@bedevere-bot bedevere-bot added the awaiting core review label Aug 15, 2023
@bedevere-bot bedevere-bot mentioned this pull request Aug 15, 2023
Modernise code in Tools/clinic/ #104683
Closed
5 of 7 tasks
@erlend-aasland

erlend-aasland commented Aug 15, 2023

Copy link
Copy Markdown
Contributor Author

This refactor helped me discover a regression of gh-107885; Argument Clinic does not fail() for these cases:

/*[clinic input]
class C "void *" ""
C.meth
  a: int
[clinic start generated code]*/
/*[clinic input]
# This should not be allowed!
C.__new__ = C.meth
[clinic start generated code]*/
/*[clinic input]
# Neither should this
@classmethod
C.__init__ = C.meth
[clinic start generated code]*/

We should bake those assertions into the parse_names helper.

AlexWaygood 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.

AlexWaygood
AlexWaygood reviewed Aug 15, 2023
View reviewed changes
Comment thread Tools/clinic/clinic.py Resolved

Uh oh!

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

Comment thread Tools/clinic/clinic.py Resolved

Uh oh!

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

@erlend-aasland

erlend-aasland commented Aug 15, 2023

Copy link
Copy Markdown
Contributor Author

Post this, I want to further extract helpers from this stage of the state machine; with this refactor, the remaining code is clearer (simply, since it is fewer lines of code), and it is now obvious we need to do something with the _module_and_class() calls :)

All reactions

Sorry, something went wrong.

Uh oh!

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

AlexWaygood
AlexWaygood reviewed Aug 15, 2023
View reviewed changes
Comment thread Tools/clinic/clinic.py Resolved

Uh oh!

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

@AlexWaygood

AlexWaygood commented Aug 15, 2023

Copy link
Copy Markdown
Member

it is now obvious we need to do something with the _module_and_class() calls :)

Off-topic, but I'm pretty sure this block of code is unreachable FYI:

cpython/Tools/clinic/clinic.py

Lines 2440 to 2441 in 13c36dc

if not hasattr(parent, 'classes'):
return module, cls

It's not currently covered by any of our tests. If our type annotations are correct, moreover (and I believe they are), parent should only ever be an instance of Clinic, Module or Class. Instances of Clinic, Module and Class all have a .classes attribute. (Mypy likely doesn't catch this because it doesn't have a great understanding of hasattr(), which is a bit too dynamic for it.)

erlend-aasland reacted with laugh emoji
All reactions
  • 😄 1 reaction

Sorry, something went wrong.

Uh oh!

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

@erlend-aasland

erlend-aasland commented Aug 15, 2023

Copy link
Copy Markdown
Contributor Author

There are some corner cases which I think should be fixed:

  • The regression from gh-107880: Teach Argument Clinic to clone __init__ and __new__ methods #107885
  • gh-104683: Argument Clinic: Extract parse_function_names() helper #107964 (comment)

I suggest we create an issue for this, and retarget this PR as a fix for these issues.

All reactions

Sorry, something went wrong.

Uh oh!

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

@erlend-aasland
Ensure a C basename is provided after 'as'
76b637a
@AlexWaygood

AlexWaygood commented Aug 15, 2023

Copy link
Copy Markdown
Member

The knot of code in #107964 (comment) is such a complex conditional (and, it appears, apparently buggy as well) that I think I'd prefer to see a dedicated PR untangling just that bit, and fixing the bug, before we go ahead with a change that factors out the common logic into a helper function. Would that be okay?

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.

@erlend-aasland
Add test_no_c_basename()
0243b09
@erlend-aasland
erlend-aasland marked this pull request as draft August 15, 2023 12:13
@bedevere-bot bedevere-bot removed the awaiting core review label Aug 15, 2023
This was referenced Aug 15, 2023
Argument Clinic silently allow empty custom C basenames #107972
Closed
gh-107880: Argument Clinic: Fix regression in gh-107885 #107974
Merged
erlend-aasland added 4 commits August 15, 2023 16:21
@erlend-aasland
Pull in main
f2aefff
@erlend-aasland
Pull in main
06d4f62
@erlend-aasland
Pull in main
4620731
@erlend-aasland
Adjust test
3f63df9
@erlend-aasland
erlend-aasland marked this pull request as ready for review August 15, 2023 21:04
@bedevere-bot bedevere-bot added the awaiting core review label Aug 15, 2023
AlexWaygood
AlexWaygood reviewed Aug 16, 2023
View reviewed changes
Comment thread Tools/clinic/clinic.py Resolved

Uh oh!

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

AlexWaygood
AlexWaygood reviewed Aug 16, 2023
View reviewed changes
Comment thread Tools/clinic/clinic.py Resolved

Uh oh!

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

erlend-aasland and others added 3 commits August 16, 2023 13:19
@erlend-aasland @AlexWaygood
Style
2e68f63
Co-authored-by: Alex Waygood <[email protected]>
@erlend-aasland
Pull in main
df8a2cc
@erlend-aasland
Use a named tuple
a915dc9
AlexWaygood
AlexWaygood reviewed Aug 16, 2023
View reviewed changes
Comment thread Tools/clinic/clinic.py Resolved

Uh oh!

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

@erlend-aasland @AlexWaygood
Update Tools/clinic/clinic.py
3735c95
Co-authored-by: Alex Waygood <[email protected]>
AlexWaygood
AlexWaygood approved these changes Aug 16, 2023
View reviewed changes

@AlexWaygood AlexWaygood left a comment

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.

Thanks!

Sorry, something went wrong.

Uh oh!

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

erlend-aasland reacted with hooray emoji
All reactions
  • 🎉 1 reaction
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting core review labels Aug 16, 2023
AlexWaygood
AlexWaygood reviewed Aug 16, 2023
View reviewed changes

@AlexWaygood AlexWaygood left a comment

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.

Oh, one optional other suggestion:

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread Tools/clinic/clinic.py Resolved

Uh oh!

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

Comment thread Tools/clinic/clinic.py Resolved

Uh oh!

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

Comment thread Tools/clinic/clinic.py Resolved

Uh oh!

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

@erlend-aasland @AlexWaygood
Improved function name
2ee0340
Co-authored-by: Alex Waygood <[email protected]>
@erlend-aasland
erlend-aasland enabled auto-merge (squash) August 16, 2023 13:01
@erlend-aasland

erlend-aasland commented Aug 16, 2023

Copy link
Copy Markdown
Contributor Author

Thanks for the review!

AlexWaygood reacted with rocket emoji
All reactions
  • 🚀 1 reaction

Sorry, something went wrong.

Uh oh!

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

AlexWaygood
AlexWaygood approved these changes Aug 16, 2023
View reviewed changes

@AlexWaygood AlexWaygood left a comment

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.

🎉

Sorry, something went wrong.

Uh oh!

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

All reactions
@AlexWaygood AlexWaygood changed the title gh-104683: Argument Clinic: Extract parse_names() helper gh-104683: Argument Clinic: Extract parse_function_names() helper Aug 16, 2023
@erlend-aasland
erlend-aasland merged commit 42429d3 into python:main Aug 16, 2023
@bedevere-bot bedevere-bot removed the awaiting merge label Aug 16, 2023
@erlend-aasland
erlend-aasland deleted the clinic/extract-parse-names-helper branch August 16, 2023 13:50
@erlend-aasland

erlend-aasland commented Aug 16, 2023

Copy link
Copy Markdown
Contributor Author

I'll follow-up with further refactorings in this part of the source code; there's still a lot of duplicated code there.

All reactions

Sorry, something went wrong.

Uh oh!

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@AlexWaygood AlexWaygood AlexWaygood approved these changes

Uh oh!

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

Assignees

No one assigned

Labels

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.

3 participants

@erlend-aasland @AlexWaygood @bedevere-bot
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.