GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||
GeezerVison is showing github.com — gh-104683: Argument Clinic: Extract parse_function_names() helper by erlend-aasland · Pull Request #107964 · python/cpython Conversation
|
|
This refactor helped me discover a regression of gh-107885; Argument Clinic does not /*[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 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
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 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
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
It's not currently covered by any of our tests. If our type annotations are correct, moreover (and I believe they are), |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
There are some corner cases which I think should be fixed:
I suggest we create an issue for this, and retarget this PR as a fix for these issues. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
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? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Co-authored-by: Alex Waygood <[email protected]>
There was an error while loading. Please reload this page.
Co-authored-by: Alex Waygood <[email protected]>
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Co-authored-by: Alex Waygood <[email protected]>
|
Thanks for the review! |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
🎉
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
I'll follow-up with further refactorings in this part of the source code; there's still a lot of duplicated code there. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.