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

Execute command support #274

Merged
merged 5 commits into from
Oct 21, 2020
Merged

Execute command support #274

merged 5 commits into from
Oct 21, 2020

Conversation

appilon
Copy link
Contributor

@appilon appilon commented Oct 9, 2020

Add support for command execution (https://microsoft.github.io/language-server-protocol/specifications/specification-current/#workspace_executeCommand). First command is rootmodules which given a file uri reports if the walker is still walking and the root modules the file is associated to. Multiple rootmodule associations can occur if the file in focus is a module definition for multiple projects.

This PR establishes some design for requests which should pass "var=value" string arguments (prevent position from causing breaking changes), variables are case insensitive, values are interpreted as float64, then bool and fallback to string, they can be accessed by the commandArgs.Get<Type> methods.

Responses are a JSON object which is what the spec supports, they should have a version attribute starting at zero in case there is a need for breaking changes down the road.

Reports rootmodules associated to passed file
Implemented var=value style argument handling for executeCommand, and
added version attribute to response for future flexibility. Improved
diagnostic push error handling (don't panic on connection closed).
Add test cases for command handlers.
@appilon appilon marked this pull request as ready for review October 16, 2020 00:05
@appilon appilon requested review from a team and paultyng October 16, 2020 00:05
@appilon appilon force-pushed the executeCommand branch 2 times, most recently from 53a4b6b to 5806e33 Compare October 16, 2020 18:32
.go-version Outdated Show resolved Hide resolved
internal/terraform/rootmodule/root_module.go Outdated Show resolved Hide resolved
langserver/diagnostics/diagnostics.go Outdated Show resolved Hide resolved
langserver/handlers/execute_command.go Outdated Show resolved Hide resolved
langserver/handlers/execute_command_rootmodules.go Outdated Show resolved Hide resolved
langserver/handlers/execute_command_rootmodules.go Outdated Show resolved Hide resolved
langserver/handlers/execute_command_rootmodules.go Outdated Show resolved Hide resolved
langserver/handlers/symbols.go Show resolved Hide resolved
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.

This looks good to me besides that one comment about error/panic handling and that one failing test, which probably just needs test data updated.

@appilon
Copy link
Contributor Author

appilon commented Oct 21, 2020

All comments addressed and tests passing, taking last review as approval

@appilon appilon merged commit e03db27 into master Oct 21, 2020
@appilon appilon deleted the executeCommand branch October 21, 2020 19:09
@radeksimko radeksimko added this to the v0.9.0 milestone Nov 4, 2020
@ghost
Copy link

ghost commented Nov 20, 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 as resolved and limited conversation to collaborators Nov 20, 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.

2 participants