-
-
Notifications
You must be signed in to change notification settings - Fork 47
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
Fix/callable protocols 2 #599
base: main
Are you sure you want to change the base?
Changes from all commits
dedbb71
e6bf056
8a868e5
a65613f
7f72c75
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -1101,14 +1101,15 @@ def adapt_typehints( | |||||||||||||||||||
|
||||||||||||||||||||
|
||||||||||||||||||||
def implements_protocol(value, protocol) -> bool: | ||||||||||||||||||||
allowed_dunder_methods = {"__call__"} | ||||||||||||||||||||
Comment on lines
1102
to
+1104
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Better to have the set as a global, instead of inside the function. |
||||||||||||||||||||
from jsonargparse._parameter_resolvers import get_signature_parameters | ||||||||||||||||||||
from jsonargparse._postponed_annotations import get_return_type | ||||||||||||||||||||
|
||||||||||||||||||||
if not inspect.isclass(value): | ||||||||||||||||||||
if not inspect.isclass(value): # Should we check if callables implement protocols? | ||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It does make sense that a function could implement a callable protocol. But I would leave this for a different pull request. |
||||||||||||||||||||
return False | ||||||||||||||||||||
members = 0 | ||||||||||||||||||||
for name, _ in inspect.getmembers(protocol, predicate=inspect.isfunction): | ||||||||||||||||||||
if name.startswith("_"): | ||||||||||||||||||||
if name.startswith("_") and name not in allowed_dunder_methods: | ||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Give me some time and I analyze the dunder methods in general and maybe in this pull request we do more than There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
This is what I came up with. |
||||||||||||||||||||
continue | ||||||||||||||||||||
if not hasattr(value, name): | ||||||||||||||||||||
return False | ||||||||||||||||||||
|
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.
v4.34.0 hasn't been released yet, so this entry should be there instead of a new v4.34.1 section. Also please add a link to this pull request.