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

LSP: Include braces in type import completions #3516

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

crazymerlyn
Copy link
Contributor

Fixes #3498

Copy link
Member

@lpil lpil 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! Could you add some tests and update the changelog please 🙏

@lpil lpil marked this pull request as draft August 16, 2024 19:28
@crazymerlyn
Copy link
Contributor Author

I tried to add tests but ran into an issue. With or without my change, the tests don't generate any completions for import dep.| while working just fine for import dep.{|}. Since the behavior hasn't really changed for the second case, it's hard to add a test for it.

I did test it by installing it locally and trying it out in my editor but can't seem to replicate that behavior in the tests. Regardless of whether i use assert_completion! or assert_completion_with_prefix i can't get the completion to be generated in the case of import dep.|. Not idea why that is considering the lsp generates those completions just fine when actually using it.

@lpil
Copy link
Member

lpil commented Sep 5, 2024

Are you still working on this @crazymerlyn ?

@crazymerlyn
Copy link
Contributor Author

Haven't looked at it since my last comment. Don't really have an idea for what to do next in terms of testing this change.

@Frank-III
Copy link
Contributor

I tried to add tests but ran into an issue. With or without my change, the tests don't generate any completions for import dep.| while working just fine for import dep.{|}. Since the behavior hasn't really changed for the second case, it's hard to add a test for it.

I did test it by installing it locally and trying it out in my editor but can't seem to replicate that behavior in the tests. Regardless of whether i use assert_completion! or assert_completion_with_prefix i can't get the completion to be generated in the case of import dep.|. Not idea why that is considering the lsp generates those completions just fine when actually using it.

I think this happens because import dep.| results in a parse error and make the completion failed at this stage:

let module = match this.module_for_uri(&params.text_document.uri) {
                Some(m) => m,
                None => return Ok(None),
}; // can't get the module

probably need to figure out how to test this properly

@lpil
Copy link
Member

lpil commented Dec 2, 2024

Hello! Are you still working on this one? :)

@Frank-III
Copy link
Contributor

Hi @lpil, I would like to help finish this one as the logic is completed, we just need a way to test it. since import dep.| is not a valid syntax, the current way of testing does not apply to this one, do you have suggestions on how to test it? Appreciated!

@lpil
Copy link
Member

lpil commented Dec 6, 2024

You would need to compile it with valid syntax, update the file in the filesystem and then ask for compilations!

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.

LSP: autocompleting type imports should add brackets if needed
3 participants