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

Recursive variable expansion in lsp_execute command #1499

Closed
deathaxe opened this issue Nov 30, 2020 · 1 comment · Fixed by #1512
Closed

Recursive variable expansion in lsp_execute command #1499

deathaxe opened this issue Nov 30, 2020 · 1 comment · Fixed by #1512

Comments

@deathaxe
Copy link
Contributor

Is your feature request related to a problem? Please describe.

The documentation states ...

[
  // ...
  {
    "caption": "Thread First",
    "command": "lsp_execute",
    "args": {
      "command_name": "thread-first",
      "command_args": ["${file_uri}", 0, 0]
    }
  }
]

Note: command_args is optional depending on the workspace/executeCommand that are supported by the LSP server.
The following variables will be expanded, but only if they are top-level array items and not within nested arrays or objects:

The LemMinX language server provides a validation command which expects textDocumentIdentifie as first parameter.

see: eclipse-lemminx/lemminx#938

The proper command definition in ST would look like

[
	{
		"caption": "XML: Validate File",
		"command": "lsp_execute",
		"args": {
			"command_name": "xml.validation.current.file",
			"command_args": [{"uri": "${file_uri}"}]
		}
	}
]

Unfortunatelly ${file_uri} is not expanded as it is not in the top-level array.

Describe the solution you'd like

The most flexible and straight forward solution would probably be to support recursive variable expansions in all nested arrays and objects.

Describe alternatives you've considered

An $document_id variable which is expanded to {"uri": "file:///path/to/file.xml"} would do the job as well. The command definition would look as follows then.

[
	{
		"caption": "XML: Validate File",
		"command": "lsp_execute",
		"args": {
			"command_name": "xml.validation.current.file",
			"command_args": ["$document_id"]
		}
	}
]
@jwortmann
Copy link
Member

The way to replace some selected variables as top-level items implemented in

if arg in ["$file_uri", "${file_uri}"]:
command_args[i] = uri_from_view(self.view)
elif arg in ["$selection", "${selection}"]:
command_args[i] = self.view.substr(region)
elif arg in ["$offset", "${offset}"]:
command_args[i] = region.b
elif arg in ["$selection_begin", "${selection_begin}"]:
command_args[i] = region.begin()
elif arg in ["$selection_end", "${selection_end}"]:
command_args[i] = region.end()
elif arg in ["$position", "${position}"]:
command_args[i] = offset_to_point(self.view, region.b).to_lsp()
elif arg in ["$range", "${range}"]:
command_args[i] = region_to_range(self.view, region).to_lsp()
was used because it was just the most simple approach to do it. It would have been nice if we could use sublime.expand_variables() here to automatically search through arrays/mappings, but this isn't possible because it only works like a "string interpolation", i.e. all values would have to be strings, while in practice values can also be numbers or other valid JSON values (arrays, objects).

I'm not sure whether it would be worth to recursively search through all arrays/objects here, because I would assume that the overwhelming majority of language servers use some of the predefined structures from LSP if they expect a more complex value than some scalar int/string/boolean. And even if a recursive search would be implemented, it still isn't guaranteed that all possible values can be constructed from the variables we defined. So for this application here, I would suggest to just add another variable for textDocumentIdentifier like the suggested ${document_id}. The following lines should do it:

from .core.views import text_document_identifier

# [....]

            elif arg in ["$document_id", "${document_id}"]:
                command_args[i] = text_document_identifier(self.view)

Perhaps we should name it more similar to the actual name from LSP ${text_document_identifier}? (And mabye we better should have used the LSP name ${document_uri} for DocumentUri instead of ${file_uri}, but it's unfeasible to change it now.)

And if a server requires another kind of LSP structure as value in the future, we can easily add it when necessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants