-
-
Notifications
You must be signed in to change notification settings - Fork 31.8k
Revert __signature__ to being a cache, remove string and callable support #116110
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
Comments
Perhaps, #115984 may be an alternative implementation. But this pr was rejected. |
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. |
I think the discussion taking place should reach a conclusion before more changes are done |
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.
I feel that the discussed proposal has a very little support, so far. But more complex logic for handling the |
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. |
So, in principle, you will not object if I reopen mentioned PR as a solution for this issue? |
This is an alternative to python#100168.
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
|
Thanks.
@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. |
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 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. |
This is an alternative to python#100168.
…_ attribute" This reverts commit 1f3c7f3.
…ute (pythonGH-116234) This is an alternative to pythonGH-100168.
Feature or enhancement
Proposal:
#100039 modified the behavior of
__signature__
as used by the inspect.signature function :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
The text was updated successfully, but these errors were encountered: