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

feat(lsp): add import completions #9821

Merged
merged 11 commits into from
Mar 25, 2021

Conversation

kitsonk
Copy link
Contributor

@kitsonk kitsonk commented Mar 18, 2021

No description provided.

@kitsonk kitsonk requested a review from lucacasonato March 18, 2021 01:32
Copy link
Member

@lucacasonato lucacasonato left a comment

Choose a reason for hiding this comment

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

Implementation looks great. Just tried this out locally, and have a few remarks:

  • Only files that have previously been opened show up in completions. This is somewhat unexpected from a user point of view. I would expect that entering ./ would complete all sibling files in the directory, not just files that have previously been opened.
  • I think in a later pass it would make sense to autocomplete all remote imports that are already cached in DENO_DIR, not just ones in the current workspaces' module graph.

Feels great to use though. Completions are snappy, even on a debug build.

@kitsonk
Copy link
Contributor Author

kitsonk commented Mar 22, 2021

I think in a later pass it would make sense to autocomplete all remote imports that are already cached in DENO_DIR, not just ones in the current workspaces' module graph.

Thinking about this, the cache is a one way hash, so that would be hard.

@kitsonk kitsonk force-pushed the feat_lsp_import_completions branch from 2d78135 to e9ed4bc Compare March 22, 2021 07:33
@lucacasonato
Copy link
Member

Thinking about this, the cache is a one way hash, so that would be hard.

The meta files contain the original URL if I'm not mistaken.

@lucacasonato
Copy link
Member

Tried it out locally again and found a few more issues:

broken_completions.mp4

@kitsonk
Copy link
Contributor Author

kitsonk commented Mar 22, 2021

Thinking about this, the cache is a one way hash, so that would be hard.

The meta files contain the original URL if I'm not mistaken.

Hrmmm... opening every metafile, parsing it, and extracting the URL won't be fast. We can be smart by host, but that is as deep as it can go, and we can obviously cache it, but still...

@kitsonk
Copy link
Contributor Author

kitsonk commented Mar 22, 2021

Tried it out locally again and found a few more issues:

Ok, I think I know what is going on.

@kitsonk kitsonk force-pushed the feat_lsp_import_completions branch from e9ed4bc to 0093dd1 Compare March 23, 2021 23:28
@kitsonk
Copy link
Contributor Author

kitsonk commented Mar 23, 2021

Tried it out locally again and found a few more issues:

Ok, I think I know what is going on.

@lucacasonato ok, I think they are fixed... it was a bit of a battle to get the textEdit working with vscode, but it now works and hopefully the behaviour is consistent with other editors. If you get a chance to take it for a spin. I couldn't break it.

@kitsonk kitsonk requested a review from lucacasonato March 23, 2021 23:44
@lucacasonato
Copy link
Member

lucacasonato commented Mar 24, 2021

Tried again, and one issue is still present. I can not trigger completions in the middle of a file / folder name. For example I enter ./sr then press ctrl + space, but nothing shows up. If I enter ./src I get completions for items in the folder. I would also argue that ./src + ctrl + space should not trigger completions for items inside ./src, but ./src/ + ctrl + space should.

Screen.Recording.2021-03-24.at.00.57.51.mov

Could not reproduce the other issue anymore 👍

@kitsonk kitsonk force-pushed the feat_lsp_import_completions branch from 8ffd0dc to 38e00b9 Compare March 24, 2021 01:16
@kitsonk
Copy link
Contributor Author

kitsonk commented Mar 24, 2021

ok @lucacasonato here we go again... I think this fixes what you mentioned above.

Copy link
Member

@lucacasonato lucacasonato left a comment

Choose a reason for hiding this comment

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

Tried it again, and behaves like I expect now 💯

Awesome work on this!

cli/lsp/completions.rs Show resolved Hide resolved
@kitsonk kitsonk merged commit 5ebb401 into denoland:main Mar 25, 2021
@kitsonk kitsonk deleted the feat_lsp_import_completions branch March 25, 2021 00:13
kitsonk added a commit that referenced this pull request Mar 25, 2021
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.

2 participants