-
Notifications
You must be signed in to change notification settings - Fork 135
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
Conversation
There was a problem hiding this 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
That is fine, I would just keep that for another PR/issue later.
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. |
@radeksimko thanks for your suggestions. I have modified it, please have a look again if available |
There was a problem hiding this 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! 👍
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. |
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?