GeezerVison is showing github.com — Revert __signature__ to being a cache, remove string and callable support · Issue #116110 · python/cpython
Revert __signature__ to being a cache, remove string and callable support #116110DescriptionFeature or enhancement
Proposal:
#100039 modified the behavior of __signature__ as used by the inspect.signature function :
- if the fields contains a Signature object, it is returned by the function (that doesn't change)
- if it contains None, it is ignored (that doesn't change)
- if it contains a string, it is converted to a signature (new)
- if it contains a callable, it is called with no parameters and expected to return either a signature object or a string which will be treated as the above (new)
- if it contains any other value, a TypeError is raised (this used to include strings and callables, it doesn't anymore)
As for strings :
The __text_signature__ attribute's purpose is to pass a string to inform the signature. It's also only used for C-written functions, and iirc there's no way to use it in pure python : I understand that, but if the ability to specify a text override of inspect.signature must happen, then it should go through __text_signature__ and not __signature__.
As for callables :
The Signature object qualifies, and the signature function inspects, callables. So, if when asked for the signature of a given callable you return another callable, it would make more sense to consider the second callable's signature as the signature of the first callable, than considering the second callable to return the signature of the first. And why no parameters ? Why not pass the first callable as parameter ? Or the actual signature of the first callable ? That implementation raises a lot of questions of arbitrariness, which makes it a bad design in my opinion.
As for the purpose :
The use case, which was adding signature overrides to Enums, does not require any change to the inspect module, in fact a perfectly good alternative implementation can be found in #115937. It should also make for a considerably faster execution.
I suggest reverting these changes.
Links to previous discussion of this feature:
This was discussed in #115937 and #116086 in which it was advised to separate the question of the implementation from that of the documentation.
Linked PRs
Activityskirpichev commented on Feb 29, 2024 Perhaps, #115984 may be an alternative implementation. But this pr was rejected. Gouvernathor commented on Feb 29, 2024 ContributorAuthor More actions My sentiment reading #116086 is that it was only closed in favor of the pure-doc PR. But there was no actual argument formulated against the merits of your PR, so I think it should be merged as fixing this issue. merwok commented on Feb 29, 2024 Reacted by Erlend E. Aasland skirpichev commented on Mar 1, 2024
there was no actual argument formulated against the merits of your PR
Yes, I would appreciate, if @erlend-aasland could do this. I don't think we should reopen this pr without permission from some core developer.
My guess is, that this will introduce some incompatibility. But on another hand, the documentation now says explicitly, that we can break things here. So, I would prefer #115984 for a simpler logic.
the discussion taking place should reach a conclusion before more changes are done
https://discuss.python.org/t/signatures-for-extension-modules-pep-draft/43914
I feel that the discussed proposal has a very little support, so far. But more complex logic for handling the __signature__ attribute is not too important for one. Reacted by Erlend E. Aasland erlend-aasland commented on Mar 1, 2024
So, I would prefer #115984 for a simpler logic.
I don't have an opinion about that particular change, and I don't have the bandwidth nor desire to involve myself in such a discussion. But such a change would need to be discussed with a broader audience than on a GitHub issue. Please open a discussion on Discourse. skirpichev commented on Mar 1, 2024
I don't have an opinion about that particular change
So, in principle, you will not object if I reopen mentioned PR as a solution for this issue? erlend-aasland commented on Mar 1, 2024
[...] you will not object if I reopen mentioned PR as a solution for this issue?
I will not object. Anyone is free to open and re-open PRs. Of course, it is good practice not to repeatedly try to push a change that has been rejected1, as that is seen as pushy or demanding behaviour. I still think your change needs a wider discussion (for example on Discourse).
Footnotes
-
generally speaking, not specifically regarding your PR ↩
skirpichev commented on Mar 1, 2024
I will not object.
Thanks.
I still think your change needs a wider discussion (for example on Discourse).
@Gouvernathor opened d.p.o discussion about docs, I just left comment on possible changes in logic. I hope, it's better than starting a new thread. Reacted by Erlend E. Aasland Gouvernathor commented on Mar 1, 2024 ContributorAuthor More actions I think it's better to a new discourse thread, separate both from this issue and the pure-doc one I opened (since it's located in the documentation part of the forum, it's not really the place to discuss implementation changes).
I also think it would be better not to have two discussions in parallel, so it should probably wait for the other discussion (with the PEP draft) to settle. So, I can either open it now and present it as "on hold", or open it after the other discussion is done. Probably the former, and I'll @ you in it anyway.
I suggest you reopen your PR as a draft (so it doesn't grab attention as something to be reviewed), and link it to this issue if possible. Reacted by Erlend E. Aasland skirpichev commented on Oct 7, 2024 Reacted by Gouvernathor and Élie Goudout added a commit that references this issue on Oct 8, 2024gh-116110: remove extra processing for the __signature__ attribute (G…
MetadataMetadataStatusShow more project fields
DevelopmentNo branches or pull requestsIssue actions
|