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

Added support of tab completion via signature #411

Merged
merged 3 commits into from Jan 28, 2020
Merged

Conversation

jlstevens
Copy link
Collaborator

@jlstevens jlstevens commented Jan 27, 2020

Work in progress PR to address #405.

@coveralls
Copy link

coveralls commented Jan 27, 2020

Coverage Status

Coverage decreased (-0.2%) to 80.153% when pulling bbf6f61 on patch_signature into 856ee5e on master.

@jlstevens
Copy link
Collaborator Author

Tab completion based on __signature__ seems to be working starting at 5c46947. Not quite sure if this PR is ready yet: I had to do a fair bit of filtering on the arguments due to name clashes. I'll see if the tests pass on travis before taking another look (and probably adding a unit test or two).

for k in extra_kwargs
if k not in [p.name for p in filtered_signature]]
all_params = (filtered_signature + extra_params
+ [inspect.Parameter('kwargs', inspect.Parameter.VAR_KEYWORD)])
Copy link
Collaborator Author

@jlstevens jlstevens Jan 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that the **kwargs VAR_KEYWORD at the end here is not strictly needed for tab-completion to work. If we don't expect any other kwargs I am happy to remove it.

all_params = (filtered_signature + extra_params
+ [inspect.Parameter('kwargs', inspect.Parameter.VAR_KEYWORD)])
signature = inspect.Signature(all_params)

parameters += [(o, None) for o in
valid_opts+kind_opts+converter._axis_options+converter._op_options]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the extra_kwargs processing and this not be merged somehow? Seems like they would be duplicated here as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume you are talking about parameters in which case yes...there will be duplication in the docstring completion hint. The difference is that duplication in the docstring hint doesn't generate an error (and tab completion works fine) while the signature approach validates against duplication.

I can use unique_iterator here but I'm not sure it is worth doing anything much more complicated: we will be dropping Python 2 before long (so we won't be using the docstring hint anymore) and the only improvement would be a slight reduction in the verbosity of part of the docstring no one looks at anyway...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in bbf6f61.


extra_kwargs = _hv.core.util.unique_iterator(valid_opts + kind_opts
+ converter._axis_options
+ converter._op_options)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't have to change this now but I've adopted this indentation style (which conforms to blacks line wrapping rules) for Panel and hvPlot now:

extra_kwargs = _hv.core.util.unique_iterator(
    valid_opts + kind_opts + converter._axis_options + converter._op_options
)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in bbf6f61.

@philippjfr philippjfr merged commit 1b47e3c into master Jan 28, 2020
@philippjfr
Copy link
Member

Thanks, looks good.

@maximlt maximlt deleted the patch_signature branch November 18, 2021 08:32
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.

None yet

3 participants