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 specify exclude root module path #251

Merged
merged 3 commits into from
Aug 5, 2020

Conversation

njuCZ
Copy link
Contributor

@njuCZ njuCZ commented Jul 30, 2020

fix #250

not support glob pattern yet.

another question: if users change the settings in editor side, for now the vscode extension would remind user to reload. But I wonder implementing configurationChanged event in language server side might be a better way?

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.

Thank you for the PR.

Aside from my inline comments we'll also need to document this new option in https://github.com/hashicorp/terraform-ls/blob/master/docs/SETTINGS.md

langserver/handlers/initialize.go Outdated Show resolved Hide resolved
langserver/handlers/initialize.go Outdated Show resolved Hide resolved
internal/settings/settings.go Show resolved Hide resolved
internal/terraform/rootmodule/walker.go Outdated Show resolved Hide resolved
@radeksimko
Copy link
Member

not support glob pattern yet.

That is fine, I would just keep that for another PR/issue later.

another question: if users change the settings in editor side, for now the vscode extension would remind user to reload. But I wonder implementing configurationChanged event in language server side might be a better way?

Yes - this is the long-term plan.

Updating the configuration alone would be easy, but applying it is not as trivial in practice, because e.g. the walker currently expects just a single run per session, secondly we have to discuss the UX around this - e.g. how do we know the user has added new, yet undiscovered path - do we compare it against the list of known paths, when do we re-discover, what do we do with invalid config (conflicting options) etc. etc.

Again - it's something I would leave for another issue/PR and this one will also likely need some further discussion.

@njuCZ
Copy link
Contributor Author

njuCZ commented Jul 31, 2020

@radeksimko thanks for your suggestions. I have modified it, please have a look again if available

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, I just left you two mostly nitpicky comments - which I'm happy to address myself tomorrow morning, unless you get to it sooner. Then I'm happy to merge this! 👍

internal/terraform/rootmodule/path.go Outdated Show resolved Hide resolved
internal/terraform/rootmodule/walker.go Outdated Show resolved Hide resolved
@radeksimko radeksimko merged commit 66ab417 into hashicorp:master Aug 5, 2020
@ghost
Copy link

ghost commented Sep 4, 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 Sep 4, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow exclusion of paths in root module discovery
2 participants