-
-
Notifications
You must be signed in to change notification settings - Fork 74
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
Remove inferred docstring signature and add basic __signature__ support #802
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
maximlt
changed the title
remove inferred docstring signature
Remove inferred docstring signature
Jul 30, 2023
maximlt
changed the title
Remove inferred docstring signature
Remove inferred docstring signature and add basic __signature__ support
Jul 30, 2023
philippjfr
reviewed
Jul 31, 2023
maximlt
commented
Aug 1, 2023
Co-authored-by: Maxime Liquet <[email protected]>
Made some minor changes:
|
philippjfr
reviewed
Aug 1, 2023
philippjfr
reviewed
Aug 1, 2023
philippjfr
approved these changes
Aug 1, 2023
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 good and I'm happy with this conservative approach to auto-generating signatures!
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.
Fixes #371 by:
params(x=Number, name=String)
) as IPython would no longer leverage it for parameter autocompletion (as noted in the issue) and because it was wrong when the class was having an__init__
method with a signature different than__init__(self, **params)
Parameterized.__signature__
as a class property that returns aSignature
built from the class Parameters, only when the class (or its superclasses) doesn't implement an__init__
method that is different than the default one__init__(self, **params)
.Somewhat surprisingly 2) seemed to have broken only a few tests in Panel and none in HoloViews. The two unit tests that broke in Panel are https://github.com/holoviz/panel/blob/df0b277dca2fd54dd2ae016159adc5c33b61e358/panel/tests/widgets/test_base.py#L25 and https://github.com/holoviz/panel/blob/df0b277dca2fd54dd2ae016159adc5c33b61e358/panel/tests/test_viewable.py#L40, I believe they test that the
__signature__
of some Panel objects is actually not modified, as there is some code in Panel that modifies the signature of components. That code in Panel by the way doesn't seem to be tested, given https://github.com/holoviz/panel/blob/df0b277dca2fd54dd2ae016159adc5c33b61e358/panel/tests/conftest.py#L32. Anyway, it seems to me the change done in this PR would not affect Panel, let's double check that however if it's not tested.I have chosen not to even try to handle custom
__init__
in 2) as I'm not even sure it's possible (@jlstevens raised some good points in the issue) and I believe that a better approach for Param in the future would be to add Parameter attributes to control how the Parameter appears in the signature (e.g. positional, hidden) and provide a post init hook so that users don't have to create an__init__
in most cases.