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

context: Introduce LanguageId #1412

Merged
merged 1 commit into from
Sep 19, 2023
Merged

context: Introduce LanguageId #1412

merged 1 commit into from
Sep 19, 2023

Conversation

radeksimko
Copy link
Member

@radeksimko radeksimko commented Sep 18, 2023

Context

This is expected to be no-op for end-users but it is a small piece of code which will allow us to make further optimisations within #1358 and enable some future feature work.

Specifically:

  • Provide diagnostics in tfvars files #1410
  • Update ParseModuleConfiguration to only parse changed file (*.tf) #1404
  • We could also use this to avoid scheduling some jobs conditionally. e.g. if the job is being scheduled because *.tfvars/*.tfvars.json file has been opened or changed, we don't need to run any module (*.tf/*.tf.json) related jobs.
    • Note that the situation isn't as simple in the other direction though - i.e. we still do need to (re)run jobs related to *.tfvars/*.tfvars.json when *.tf/*.tf.json files change, because these inform the schema of the variable files.

Implementation Notes

You may ask Why IsLanguageId() bool interface specifically?

  • We could have gone with (string, bool) or (string, error) return values.
  • It is expected that the language ID will only be available in textDocument/didOpen and textDocument/didChange, rather than in all requests, hence the simple single string return value doesn't seem appropriate, whether we return empty string or panic in the edge case.
  • Having two return values means more LOC on the consumer side, i.e. one has to first check the ok/error and then compare for equality. I am assuming that we can treat other requests the same as mismatch and that the additional LOC aren't therefore useful.

@radeksimko radeksimko added the enhancement New feature or request label Sep 18, 2023
@radeksimko radeksimko self-assigned this Sep 18, 2023
@radeksimko radeksimko marked this pull request as ready for review September 18, 2023 17:13
@radeksimko radeksimko requested a review from a team as a code owner September 18, 2023 17:13
Copy link
Member

@dbanck dbanck left a comment

Choose a reason for hiding this comment

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

Why do you use a string instead of ilsp.LanguageID in the function signatures? I guess it's unlikely that the string "terraform" will ever change, but it feels cleaner to me to use the constant.

With the new context methods here, can we get rid of https://github.com/hashicorp/terraform-ls/blob/main/internal/decoder/context.go?

@radeksimko
Copy link
Member Author

@dbanck I'm not against it but it goes much further than I was hoping to go with the PR due to conflicts.

Meaning, that we should update it here

and anywhere downstream from there + in the didOpen handler we should also add a switch statement rather than be casting an arbitrary string to the desired type.

That is to say it's the right thing to do but I'm not sure it's worth doing right now due to the long-running branch. 😅

@radeksimko radeksimko merged commit 94e47bd into main Sep 19, 2023
@radeksimko radeksimko deleted the f-ctx-lang-id branch September 19, 2023 14:08
@github-actions
Copy link

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 Oct 20, 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 this pull request may close these issues.

2 participants