Skip to content
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
merged 6 commits into from
Aug 1, 2023

Conversation

maximlt
Copy link
Member

@maximlt maximlt commented Jul 29, 2023

Fixes #371 by:

  1. removing the automatically inferred docstring signature (e.g. 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)
  2. implementing Parameterized.__signature__ as a class property that returns a Signature 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).
image

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.

@maximlt maximlt changed the title remove inferred docstring signature Remove inferred docstring signature Jul 30, 2023
@maximlt maximlt changed the title Remove inferred docstring signature Remove inferred docstring signature and add basic __signature__ support Jul 30, 2023
@maximlt maximlt requested review from jlstevens and philippjfr July 30, 2023 22:42
param/parameterized.py Outdated Show resolved Hide resolved
@philippjfr
Copy link
Member

Made some minor changes:

  • Pull out the default signature into a global variable
  • Cache the generated signature on ._param__private

Copy link
Member

@philippjfr philippjfr left a 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!

@maximlt maximlt merged commit 365426f into main Aug 1, 2023
@maximlt maximlt deleted the parameterized_signature branch August 1, 2023 18:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use __signature__ to generate Parameterized signatures
3 participants