Sorry, something went wrong.
There was a problem hiding this comment.
Sorry, something went wrong.
|
|
||
| /*[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?
Sorry, something went wrong.
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?
Sorry, something went wrong.
There was a problem hiding this comment.
In the code the default is equivalent to None. I did not look deeper. @ericsnowcurrently?
Sorry, something went wrong.
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.
Sorry, something went wrong.
|
I think that in long term, it would be better to rename "restricted" to "restrict" and get rid of "restrict as restricted". 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 _PyInterpreterState_WHENCE_STDLIB whence value & failing otherwise, so I would suggest it is more correct to use 'restrict' as the argument name. This would make sense as in we restrict .call() to only operate on interpreters from the stdlib module. Should we make the change in this PR? _interpreters is a private module, and we can't backport these AC changes anyway due to the ABI breakage (which is annoying, I had also wanted to backport). A |
Sorry, something went wrong.
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. 👍
Sorry, something went wrong.
|
Thank you for the reviews @serhiy-storchaka! A |
Sorry, something went wrong.
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.