Uh oh!
There was an error while loading. Please reload this page.
GH-137630: Convert _interpreters to use Argument Clinic - #137631
Conversation
serhiy-storchaka
left a comment
There was a problem hiding this comment.
- Many keyword-or-positional parameters were made positional-only.
- Incorrect names of some parameters.
- Mark up does not work here.
Uh oh!
There was an error while loading. Please reload this page.
| /*[clinic input] | ||
| _interpreters.create | ||
| config as configobj: object(py_default="'isolated'") = NULL |
There was a problem hiding this comment.
Isn't the default value None?
There was a problem hiding this comment.
Per the docstring, "The default is 'isolated'.". I think showing this is more helpful than None, what do you think?
There was a problem hiding this comment.
In the code the default is equivalent to None. I did not look deeper. @ericsnowcurrently?
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
serhiy-storchaka
left a comment
There was a problem hiding this comment.
I think that in long term, it would be better to rename "restricted" to "restrict" and get rid of "restrict as restricted". That syntax is used to avoid name conflicts (with C keywords and other variable) and too large diffs, but if the parameter is only used once and does not conflict with other names, it is worth to rename it.
I would prefer to backport this to 3.14, but it is too late to break ABI.
AA-Turner
commented
Aug 11, 2025
I agree. It would be good to have clarity on the status as a verb or a noun, though -- is the parameter taking an action to restrict an interpreter, or is it communicating that the interpreter is already restricted. The 'restricted' arguments were introduced via #117490. The only effect at the C level is checking for a Should we make the change in this PR? A |
serhiy-storchaka
left a comment
There was a problem hiding this comment.
I meant renaming the C variable to match the parameter name.
Anyway, this is not so important. LGTM. 👍
AA-Turner
commented
Aug 12, 2025
Thank you for the reviews @serhiy-storchaka! A |
cc @ericsnowcurrently@serhiy-storchaka
Changes split up into commits, one per module-level function. If this PR is too large, happy to break it up into smaller pieces. The only unconverted function is
_interpreters.new_config(), which AC does't support.The
pydocdiff is below._interpretersto Argument Clinic #137630