Add workspace folder variable resolution for settings url#3442
Add workspace folder variable resolution for settings url#344204kash wants to merge 1 commit intoeclipse-jdtls:mainfrom
Conversation
Signed-off-by: Kashish Mittal <kmittal@redhat.com>
rgrunber
left a comment
There was a problem hiding this comment.
Overall, seems fine. My thinking on the topic has changed a bit since I made the comment at #2529 (comment) but still open to merging this. Specifically, I realized that if the client passes a set of system properties / environment variables to the JDT-LS startup, then those keys will be resolved when used in the form ${...} within a path/uri :
Does that help at all with your motivation behind making this PR ?
Also, would you be able to add a testcase ? Probably in JavaSettingsTest is a good place for it.
|
|
||
| public Preferences setFormatterUrl(String formatterUrl) { | ||
| this.formatterUrl = ResourceUtils.expandPath(formatterUrl); | ||
| this.formatterUrl = this.resolveWorkspaceFolder(ResourceUtils.expandPath(formatterUrl)); |
There was a problem hiding this comment.
If we take this approach, I would probably put the method to be called at the bottom of ResourceUtils.expandPath(..). That way, it would apply not just to formatter settings, but any attempt to resolve a path that the client has set. I guess you would need to pass in getRootPaths() as well to perform the logic there.
|
|
||
| // If no match found in any workspace folder, fall back to first workspace | ||
| IPath firstWorkspace = rootPaths.iterator().next(); | ||
| return filePath.replace(workspaceFolderVariable, firstWorkspace.toOSString()); |
There was a problem hiding this comment.
I would just return filePath if nothing exists on any of the root paths.
| } | ||
|
|
||
| private String resolveWorkspaceFolder(String filePath) { | ||
| final String workspaceFolderVariable = "${workspaceFolder}"; |
There was a problem hiding this comment.
I would use WORKSPACE_FOLDER_VAR just to make it a bit more obvious that it's constant.
This PR adds support for resolving
${workspaceFolder}in the following settings:If
${workspaceFolder}is present in the provided path, it will be replaced with the absolute path of the workspace folder (retrieved fromgetRootPaths()). The resolution logic checks all available workspace folders and uses the first matching path that exists.Fixes: #2529