-
-
Notifications
You must be signed in to change notification settings - Fork 32k
bpo-36540: PEP 570 -- Implementation #12701
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
Conversation
ea3de2e
to
08f02c6
Compare
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.
Some comments here from a very cursory pass over this.
Is the documentation for this intended to be part of a separate PR? Either way, feel free to ping me for a review when it's available.
19b27b3
to
7e77d2f
Compare
We already raise if you pass keyword only or an annotation for that API: https://github.com/python/cpython/blob/master/Lib/inspect.py#L1092 |
That was my proposal. IMHO it's the safest option for "legacy" functions of the inspect module, to ensure that we don't introduce any subtle backward-incompatible change. I modified the warnings module to add a new "source" parameter at the end. It took me 3 years to fix regressions on corner cases... The latest one was be7c460 See https://bugs.python.org/issue26568 and https://bugs.python.org/issue35178 I tried to have the backward-compatibility in mind when I modified the warnings module, but there are always subtle issues... IMHO getargs(), getargspec(), getfullargspec() should be deprecated in favor of signature() which is future-proof. But Nick Coghlan undeprecated these functions in 2017 https://bugs.python.org/issue20438 whereas these were deprecated since 2015. I didn't understand the subtle issues. Again, that's why I suggest to open an issue afterwards to discuss the subtle inspect API :-) |
Oh, then disregard what I said. |
As a side comment, Cython is already adding support for this PEP 🎉 : |
@pablogsal: Please merge your PR. Serhiy Storchaka complained about backward incompatible changes on the inspect module: But I made the same complain and you fixed it. IHMO the change is now good enough to be merged. Serhiy said nothing for 2 weeks. alpha4 release is today. I would prefer to get this implementation merged as soon as possible: as Lukasz if I understand correctly. |
My main complain was about the inspect module. I have no time to make a review of other parts, but I do not expect serious problems here. Feel free to commit. We can fix minor issues after merging if there are any. |
@@ -1185,7 +1193,7 @@ def getfullargspec(func): | |||
defaults = None | |||
|
|||
return FullArgSpec(args, varargs, varkw, defaults, |
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.
This is breaking change. getfullargspec()
was added to avoid breaking old getargspec()
. You need to add a new function (getfullerargspec()
?) and deprecate getfullargspec()
.
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 suspect that there may be a longer discussion for these cases so I opened https://bugs.python.org/issue36751 to track it and decide there.
@@ -1216,7 +1224,8 @@ def _formatannotation(annotation): | |||
return _formatannotation | |||
|
|||
def formatargspec(args, varargs=None, varkw=None, defaults=None, | |||
kwonlyargs=(), kwonlydefaults={}, annotations={}, | |||
posonlyargs=(), kwonlyargs=(), kwonlydefaults={}, |
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.
Since all these parameters are positional-or-keyword, inserting a new parameter in the middle is breaking change. You can add it only to the end. Or add a new function.
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 suspect that there may be a longer discussion for these cases so I opened https://bugs.python.org/issue36751 to track it and decide there.
Did you see that the PR has been modified to no longer touch the existing API, except of signature(), but raise an exception of the "legacy functions" are called on a function which has positional-only arguments? #12701 (comment) |
|
False positive. It is some problem in the multiprocessing module. |
@@ -102,7 +103,7 @@ PyAPI_DATA(PyTypeObject) PyCode_Type; | |||
|
|||
/* Public interface */ | |||
PyAPI_FUNC(PyCodeObject *) PyCode_New( | |||
int, int, int, int, int, PyObject *, PyObject *, | |||
int, int, int, int, int, int, PyObject *, PyObject *, |
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.
Adding a required argument to the code type constructor will breaking every library that creates types.CodeType
instances. Are you sure this is a good idea?
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.
Looks like it has already started: https://www.google.com/search?q=TypeError%3A+code%28%29+%22takes+at+least+14+arguments+%2813+given%29%22
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.
You missed the end of the story:
commit 4a2edc34a405150d0b23ecfdcb401e7cf59f4650
Author: Pablo Galindo <[email protected]>
Date: Mon Jul 1 11:35:05 2019 +0100
bpo-37221: Add PyCode_NewWithPosOnlyArgs to be used internally and set PyCode_New as a compatibility wrapper (GH-13959)
Add PyCode_NewEx to be used internally and set PyCode_New as a compatibility wrapper
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.
Also, types.CodeType
's constructor is considered private. We now even have this warning in the docs (https://docs.python.org/3.8/library/types.html#standard-interpreter-types):
If you instantiate any of these types, note that signatures may vary between Python versions.
Additionally, you have now available types.CodeType.replace
in case you can to create a copy with some fields changed.
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.
Thank you for the explanation!
I hope in future we'll have a more stable API for dynamic function generation.
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.
This breaks pickle.loads
for functions constructed as
def constant_fn(val: float) -> Callable:
def func(_):
return val
return func
fun = constant_fn(1.)
and pickled using cloudpickle
under Python 3.7; these can't be loaded back up under Python 3.8.
Should this be addressed downstream in cloudpickle ?
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.
Should this be addressed downstream in cloudpickle ?
Yes, they need to update whatever code they are using to create code objects as code objects in Python3.8 require an extra field. Please, check these sections in the docs:
https://docs.python.org/3/library/types.html#standard-interpreter-types
If you instantiate any of these types, note that signatures may vary between Python versions.
Also, check this new C-API function:
https://docs.python.org/3/c-api/code.html#c.PyCode_NewWithPosOnlyArgs
and this new convenience function:
https://docs.python.org/3/library/types.html#types.CodeType.replace
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.
Alright, I'll file an issue there. Thanks.
implementation of PEP 570 -- Python Positional-Only Parameters:
https://www.python.org/dev/peps/pep-0570/
https://bugs.python.org/issue36540
CC: @mariocj89