-
-
Notifications
You must be signed in to change notification settings - Fork 32.5k
GH-137630: Convert _interpreters
to use Argument Clinic
#137631
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
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.
- Many keyword-or-positional parameters were made positional-only.
- Incorrect names of some parameters.
- Mark up does not work here.
@@ -841,17 +848,29 @@ Any keyword arguments are set on the corresponding config fields,\n\ | |||
overriding the initial values."); | |||
|
|||
|
|||
/*[clinic input] | |||
_interpreters.create | |||
config as configobj: object(py_default="'isolated'") = NULL |
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.
Isn't the default value None
?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the code the default is equivalent to None. I did not look deeper. @ericsnowcurrently?
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.
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.
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
pydoc
diff is below._interpreters
to Argument Clinic #137630