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.

Sign up for GitHub

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

Jump to bottom

Add hooks for dynamic completion #1017

Merged
merged 10 commits into from
Aug 2, 2022
Merged

Add hooks for dynamic completion #1017

merged 10 commits into from
Aug 2, 2022

Conversation

dbanck
Copy link
Member

@dbanck dbanck commented Jul 27, 2022

This PR introduces a new hooks package, which we can use to implement hooks for dynamic completion. Hooks will be implemented as part of hashicorp/vscode-terraform#672.

We also introduce a new completionItem/resolve handler, which editors can use to update a completion item with more detailed data. The data is obtained via the execution of a resolve hook.

@dbanck dbanck added the enhancement New feature or request label Jul 27, 2022
@dbanck dbanck self-assigned this Jul 27, 2022
@dbanck dbanck force-pushed the f-hooks-for-dynamic-completion branch from a6e0712 to da74524 Compare July 28, 2022 10:56
@dbanck dbanck marked this pull request as ready for review July 28, 2022 12:25
@dbanck dbanck requested a review from a team as a code owner July 28, 2022 12:25
radeksimko
radeksimko approved these changes Jul 29, 2022
View reviewed changes
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.

LGTM, aside from a few in-line comments, although you may want to look at the upstream hcl-lang PR first and then rebase here once merged.

internal/decoder/decoder.go Outdated Show resolved Hide resolved
internal/hooks/hooks.go Outdated Show resolved Hide resolved
internal/hooks/hooks.go Outdated Show resolved Hide resolved
internal/hooks/hooks.go Outdated Show resolved Hide resolved
internal/hooks/module_source_local.go
"github.com/zclconf/go-cty/cty"
)

func (h *Hooks) LocalModuleSources(ctx context.Context, value cty.Value) ([]decoder.Candidate, error) {
Copy link
Member

Choose a reason for hiding this comment

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

I'm assuming this would be eventually part of a separate PR? 👀

Copy link
Member Author

Choose a reason for hiding this comment

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

I added this as a placeholder to showcase the registration of hooks in AppendCompletionHooks. Without any hooks we wouldn't be able to create hooks.Hooks{}.

But I can remove it if you want? And defer everything to the next PR

internal/langserver/handlers/completion_resolve.go Outdated
lsp "github.com/hashicorp/terraform-ls/internal/protocol"
)

type CompletionItem struct {
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: We could move this under the protocol package as we already have some extensions of the original structs there for telemetry or experimental functions:
https://github.com/hashicorp/terraform-ls/tree/main/internal/protocol

internal/lsp/completion.go Outdated Show resolved Hide resolved
@radeksimko radeksimko mentioned this pull request Aug 1, 2022
Merged
@dbanck dbanck force-pushed the f-hooks-for-dynamic-completion branch from d52944d to 183acf2 Compare August 1, 2022 15:24
@dbanck dbanck requested a review from radeksimko August 1, 2022 15:25
radeksimko
radeksimko approved these changes Aug 1, 2022
View reviewed changes
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.

LGTM, aside from that one naming suggestion.

As for the hooks package and removal of the actual hooks - I'll leave that at your own discretion - as per Slack convo.

internal/protocol/completion.go Outdated

import "github.com/hashicorp/hcl-lang/lang"

type CompletionItemR struct {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
type CompletionItemR struct {
type CompletionItemWithResolveHook struct {

Just for some extra clarity

@dbanck dbanck force-pushed the f-hooks-for-dynamic-completion branch from d67e2c3 to 41f2bbf Compare August 1, 2022 16:53
@radeksimko radeksimko added this to the v0.29.0 milestone Aug 1, 2022
@dbanck dbanck merged commit 33d377d into main Aug 2, 2022
@dbanck dbanck deleted the f-hooks-for-dynamic-completion branch August 2, 2022 08:01
@github-actions
Copy link

github-actions bot commented Sep 2, 2022

I'm going to lock this pull request 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 related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 2, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@radeksimko radeksimko radeksimko approved these changes

Assignees

@dbanck dbanck

Labels
enhancement New feature or request
Projects
None yet
Milestone
v0.29.0
Development

Successfully merging this pull request may close these issues.

Introduce hooks for dynamic completion
2 participants
@dbanck @radeksimko