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

Add a plugin to provide autoimport functionality #199

Merged
merged 45 commits into from
Nov 3, 2022

Conversation

bagel897
Copy link
Contributor

@bagel897 bagel897 commented Apr 18, 2022

Autoimport plugin for pylsp

Depends on accompanying pull request in rope
@bagel897
Closes #34

Prereqs

Rope > 1.0.0 (AKA git builds rn)

Demo Images

image
can detect external modules
image
grabs names from the external modules
image

Features

  • Score-based sorting algorithm with thresholding. Picks the 25 best options.
    Will take suggestions on how to improve the basic algorithm
  • Relatively high preformance using sqllite3 and ast.
    Besides the cache generation, it preforms fine on my machine
  • Avoids suggestions on single line comments, from/normal imports, function/classes, dots
    Most of the test cases are to cover this and it should be pretty functional.
  • Most completion related tests should be in rope
    the main ones are in parsing existing names and whether to suggest imports
  • inserts all imports at the bottom of the set of imports. Recommendation is to use the isort extension.
  • Watches for changes (but not deleted files) and regenerates cache for saved files.

Won't fix

  1. Correct detection of type of statements from __init__.py modules. Would be difficult to preform type inference on.
    image
  2. Doesn't handle deleted files but this is a relatively rare.
  3. Without a progress indicator, the user won't know when its ready (201) I have a WIP implementation based on Add progress reporting #236
  4. Doesn't avoid suggestions on multiple line comments/strings, can fix if deemed important

CONFIGURATION.md Outdated Show resolved Hide resolved
Other smaller changes:
 - Allow memory only database for testing.
 - Configuration parameter for memory database
@bagel897
Copy link
Contributor Author

so this is almost ready:

  1. It needs a fix for windows in rope Fix windows python-rope/rope#477 and associated release
  2. Its still a little rough around where to provide suggestions and slow when it does. Ideally, some of this can be used in jedi and rope completions.
  3. It generates a large cache which takes a while. Normally, this will be on-disk and I will work on improving it. But in CI, this takes a while and must be done seperately for each test and kept in memory. Either I change the scope of the associated fixtures to a module scope, or we have 2m long testing runs. In the future, I'll make it use configured dependencies automatically.

@bagel897
Copy link
Contributor Author

Some notes about the future of this. I'm working on splitting it out to its own repository where 1-3 things will happen.

  1. Rope-autoimport will use autoimport-core under the hood
  2. Pylsp will use autoimport-core directly
  3. Jedi will use autoimport-core, negating the requirements for any changes in pylsp.

But for now, this is perfectly functional code

@bagel897 bagel897 marked this pull request as ready for review June 22, 2022 23:33
@bagel897
Copy link
Contributor Author

Yes, but its probably better to mark it off by default, and explain that it is experimental unless it is replaced in jedi/rope

@ccordoba12
Copy link
Member

Agreed, that's fine for me too.

@mostley
Copy link

mostley commented Oct 17, 2022

Can I help somehow to make this happen?

@bagel897
Copy link
Contributor Author

Can I help somehow to make this happen?

If Jedi manifested a should_insert API, that'd help - but isn't necessary for the merge. I'm not sure what else is needed.

@ccordoba12 ccordoba12 added this to the v1.6.0 milestone Oct 17, 2022
@ccordoba12
Copy link
Member

I didn't know this was ready. I'll try to review it this week so we can include it in our next version.

Copy link
Member

@ccordoba12 ccordoba12 left a comment

Choose a reason for hiding this comment

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

Thanks a lot @bagel897 for your work on this! I left a very lightweight review for you, mostly about style issues. Otherwise looks good to me.

docs/autoimport.md Outdated Show resolved Hide resolved
pylsp/config/schema.json Outdated Show resolved Hide resolved
pylsp/plugins/rope_autoimport.py Show resolved Hide resolved
pylsp/plugins/rope_autoimport.py Outdated Show resolved Hide resolved
pylsp/workspace.py Outdated Show resolved Hide resolved
pylsp/workspace.py Outdated Show resolved Hide resolved
pylsp/workspace.py Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
test/plugins/test_autoimport.py Show resolved Hide resolved
test/plugins/test_autoimport.py Outdated Show resolved Hide resolved
@ccordoba12 ccordoba12 removed this from the v1.6.0 milestone Nov 1, 2022
@bagel897
Copy link
Contributor Author

bagel897 commented Nov 3, 2022

Pushed a bugfix and the style changes. Let me know if there's anything else (or if the CI fails, I'll check). You might want to squash the PR, since its a lot of poorly labeled commits, but up to you. Thanks for the feedback @ccordoba12!

@ccordoba12 ccordoba12 added this to the v1.7.0 milestone Nov 3, 2022
Copy link
Member

@ccordoba12 ccordoba12 left a comment

Choose a reason for hiding this comment

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

Thanks for addressing my previous review @bagel897! One last suggestion for you then this should be ready.

pylsp/plugins/rope_autoimport.py Outdated Show resolved Hide resolved
@ccordoba12
Copy link
Member

You might want to squash the PR, since its a lot of poorly labeled commits, but up to you

Don't worry about that, we squash merge all PRs in this repo.

Co-authored-by: Carlos Cordoba <ccordoba12@gmail.com>
Copy link
Member

@ccordoba12 ccordoba12 left a comment

Choose a reason for hiding this comment

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

Thanks @bagel897 for this addition! It's a great improvement!!

@ccordoba12 ccordoba12 changed the title Autoimport Add a plugin to provide autoimport functionality Nov 3, 2022
@ccordoba12 ccordoba12 merged commit b24ffd3 into python-lsp:develop Nov 3, 2022
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.

Auto-import?
5 participants