bpo-36540: PEP 570 -- Implementation #12701
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pablogsal
changed the title
pablogsal
changed the title
pablogsal
force-pushed
the
PEP570
branch
4 times, most recently
from
ea3de2e
to
08f02c6
Compare
pganssle
requested changes
Apr 7, 2019
pablogsal
force-pushed
the
PEP570
branch
4 times, most recently
from
19b27b3
to
7e77d2f
Compare
pablogsal
changed the title
pablogsal
changed the title
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.
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.
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.
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.
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. |
Ark-kun
reviewed
Feb 18, 2020
| @@ -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.
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.
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.
You missed the end of the story:
commit 4a2edc34a405150d0b23ecfdcb401e7cf59f4650
Author: Pablo Galindo <Pablogsal@gmail.com>
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.
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.
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.
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.
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.
Alright, I'll file an issue there. Thanks.
This was referenced Feb 19, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
implementation of PEP 570 -- Python Positional-Only Parameters:
https://www.python.org/dev/peps/pep-0570/
https://bugs.python.org/issue36540
CC: @mariocj89