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

Completion type hints #1785

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Conversation

cc-a
Copy link

@cc-a cc-a commented May 2, 2020

PR Summary

See #1773

This adds support for use of the get_type_hint method for Jedi completions that was added 0.17.0. For appropriate proposals type hints are added as part of the completion annotation.

Please let me know if there are any other locations where get_type_hint could be useful and I'll happily take a look.

For completion candidates of functions the type hints can be quite long so it might be sensible to introduce a truncation. Let me know any thoughts.

PR checklist

Please make sure that the following things have been addressed (and check the relevant checkboxes):

  • Commits respect our guidelines
  • Tests are passing properly (see here on how to run Elpy's tests)

For new features only:

  • Tests has been added to cover the change
  • The documentation has been updated

@coveralls
Copy link

coveralls commented May 2, 2020

Coverage Status

Coverage decreased (-0.1%) to 92.0% when pulling b6537ed on cc-a:completion-type-hints into 7e8181d on jorgenschaefer:master.

@galaunay
Copy link
Collaborator

It looks nice.

There is some tests that are failing (travis doesn't show up in PRs anymore, but here is the link).

We are still supporting python2.7 so it would be nice to make your additional test conditional.
There is another error linked to FileNotFoundError, but I am not sure where it is coming from...

@cc-a
Copy link
Author

cc-a commented May 24, 2020

Resolved the existing errors but there is a random smattering of failures. I can't see how any of them would be related to my changes and I've not been able to recreate them locally or in Docker containers. I'm open to suggestions on how to proceed.

@galaunay
Copy link
Collaborator

Unfortunately, those random failures are a long-standing problem I cannot really get my head around... I restarted the tests and your commit looks fine.

It works nicely, but it can take quite some space and become unreadable depending on the typing. For examples, for the json package on a clean Emacs, this is what I get:
img-2020-05-27-172105

Some ideas that can help reduce the clutter:

  • It would be nice to remove the spaces and newlines (with hint = re.sub('\s+', ' ', hint) or similar).
  • Maybe we don't need the statement and function annotations if we already have the type hint (should be pretty explicit ?).
  • Having the class __init__ arguments would be nice as well, but I don't know if it is doable...

Quickly trying to address the first two points, this is what I get:

def get_annotation(proposal):
    if proposal.type in ("instance", "statement", "param", "function"):
        try:
            hint = proposal.get_type_hint().replace('\n', ' ')
            hint = re.sub("\\s+", " ", hint)
            hint = re.sub("\\s*= \\.\\.\\.\\s*", " ", hint)
            hint = re.sub("\\s+,", ",", hint)
            hint = "  {}".format(hint)
        except (AttributeError, TypeError, NotImplementedError, NameError):
            hint = ""
        if hint == "":
            return proposal.type
        else:
            return hint
    else:
        return proposal.type

img-2020-05-27-175011

A bit better but still quite long. In my opinion we will need to make this feature optional (some users may not want this additional information) and maybe strip the end of the annotations to keep the company window to a decent size.

Hope it makes sens.

@galaunay
Copy link
Collaborator

Another solution would be to put the type hints as "meta" instead of "annotation".
This will display the type hints in the minibuffer (where there is more space).
It sounds like a good solution to me: it is not cluttering the company menu and still make the information accessible.

@galaunay galaunay force-pushed the master branch 4 times, most recently from c4a2564 to d974e00 Compare June 30, 2021 22:40
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.

3 participants