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 support for autocomplete based on a prefix #119

Merged
merged 4 commits into from
Jun 2, 2020

Conversation

njuCZ
Copy link
Contributor

@njuCZ njuCZ commented May 27, 2020

Related: #12

  • in your test case, when Line: 2, Column: 5, the Byte should be 21 but not 20
  • add some corner case for func TokenAtPosition
  • I just filter the results in the last step, please confirm whether we should filter the candidates before putting them into the candidate list
  • tested in vscode, it's ok for this feature

@radeksimko
Copy link
Member

in your test case, when Line: 2, Column: 5, the Byte should be 21 but not 20

Agreed - I missed that one, thank you for spotting and fixing it 👍

add some corner case for func TokenAtPosition

yep - I can work on that and push it to your branch, or in a separate PR

I just filter the results in the last step, please confirm whether we should filter the candidates before putting them into the candidate list

That's a good question, I left a comment inline about that - I think we should indeed do it before.

tested in vscode, it's ok for this feature

Me and @paultyng also tested it in VSCode ourselves and it works well except for one edge case, which is completion in the middle of an incomplete label, e.g.

resource "aws_<HERE>" "name" {
  
}

I will look into this ^

@radeksimko
Copy link
Member

radeksimko commented May 28, 2020

That previously discussed refactoring is now ready for review in #125 - that should make it easier to access tokens and implement TokenAtPos in some form down in internal/terraform/lang.

@radeksimko
Copy link
Member

#125 was merged, so this PR now may need rebasing. Let me know if you need any further help here.

@njuCZ
Copy link
Contributor Author

njuCZ commented Jun 1, 2020

@radeksimko to filter candidates before putting into the candidate list, I have done a little refactor of the codes, Could you please have a look ?

@paultyng
Copy link
Contributor

paultyng commented Jun 1, 2020

Should we remove this check now? As it stands, I can't "recomplete" an attribute if I chose the wrong name from the middle or an existing line, just a blank line:

https://github.com/hashicorp/terraform-ls/pull/119/files#diff-7f004988141686ad2e40b48a8346a3b7R76-R79

@radeksimko radeksimko changed the title add support for autocomplete based on a prefix Add support for autocomplete based on a prefix Jun 2, 2020
@radeksimko
Copy link
Member

radeksimko commented Jun 2, 2020

Thank you for the recent changes.

I've done some further refactoring as this functionality proved that the original interface/abstraction with pure tokens wasn't really sufficient.

The main motivation behind the refactoring here was to reduce and simplify the top layer (handler), decouple some generic HCL parsing logic and finally to keep CompletionCandidate immutable (as opposed to letting anyone SetPrefix) and allow passing around already "trimmed" candidates as opposed to letting the lsp package handle the trimming via PrefixRange.

I also put back some tests which were removed, as I think there's still value in keeping them and testing the new behaviour (error).

radeksimko and others added 4 commits June 2, 2020 14:15
This also helps keeping CompletionCandidate immutable
and allows passing around already "trimmed" candidates
as opposed to letting the lsp package handle the trimming.
Copy link
Member

@radeksimko radeksimko left a comment

Choose a reason for hiding this comment

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

btw. I modified the original comment to avoid closing #12 as that issue actually isn't addressed by this PR yet, but it should be much easier to address after we merge this PR.

@radeksimko radeksimko merged commit 8499f7b into hashicorp:master Jun 2, 2020
@ghost
Copy link

ghost commented Jul 2, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the context necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Jul 2, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants