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

Centralize diagnostics publishing #1337

Closed
1 task
dbanck opened this issue Jul 31, 2023 · 3 comments · Fixed by #1361
Closed
1 task

Centralize diagnostics publishing #1337

dbanck opened this issue Jul 31, 2023 · 3 comments · Fixed by #1361
Assignees
Labels
enhancement New feature or request

Comments

@dbanck
Copy link
Member

dbanck commented Jul 31, 2023

Language Server Version

v0.31.3

Problem Statement

We currently publish diagnostics from two different locations:

In a module hook

diags := diagnostics.NewDiagnostics()
diags.EmptyRootDiagnostic()
defer dNotifier.PublishHCLDiags(ctx, mod.Path, diags)
if mod != nil {
diags.Append("HCL", mod.ModuleDiagnostics.AutoloadedOnly().AsMap())
diags.Append("HCL", mod.VarsDiagnostics.AutoloadedOnly().AsMap())
}

In the didSave handler when validate on save is enabled

diags := diagnostics.NewDiagnostics()
validateDiags := diagnostics.HCLDiagsFromJSON(jsonDiags)
diags.EmptyRootDiagnostic()
diags.Append("terraform validate", validateDiags)
diags.Append("HCL", mod.ModuleDiagnostics.AutoloadedOnly().AsMap())
diags.Append("HCL", mod.VarsDiagnostics.AutoloadedOnly().AsMap())

With the introduction of early validation, we'll likely have a third location that won't have access to the diagnostics raised by terraform valdiate.


Part of hashicorp/vscode-terraform#720

Attempted Solutions

No response

Proposal

  • Rework the module hooks, that only one is publishing diagnostics

Related LSP methods

  • textDocument/didSave
  • textDocument/didChange
  • textDocument/didOpen

References

No response

Help Wanted

  • I'm interested in contributing a fix myself

Community Note

  • Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request
  • Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for issue followers and do not help prioritize the request
  • If you are interested in working on this issue or have submitted a pull request, please leave a comment
@radeksimko
Copy link
Member

This may be more related to #1339 but one challenge with notifications triggered from didSave is that didSave generally does not make document changes, and hence it won't trigger the hooks currently.

This is kind of by design (at least within the LSP) but it does mean that we may have to find some way of synchronising didSave with hooks completion, in addition to persisting diagnostics (at least before they get published?).

The reason I think it's also related to #1339 is because there is other work than just diagnostic publishing (such as telemetry sending) that could also run less frequently, maybe on save or within N seconds after the last didChange, depending on which comes first.

@dbanck dbanck self-assigned this Aug 23, 2023
@dbanck dbanck linked a pull request Aug 29, 2023 that will close this issue
@dbanck
Copy link
Member Author

dbanck commented Aug 29, 2023

Closed by #1361

@dbanck dbanck closed this as completed Aug 29, 2023
@github-actions
Copy link

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 details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 29, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants