-
Notifications
You must be signed in to change notification settings - Fork 429
Add workspace folder variable resolution for settings url #3442
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
base: main
Are you sure you want to change the base?
Add workspace folder variable resolution for settings url #3442
Conversation
Signed-off-by: Kashish Mittal <[email protected]>
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.
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 :
eclipse.jdt.ls/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/ResourceUtils.java
Lines 262 to 281 in c2cd627
StrLookup<String> variableResolver = new StrLookup<>() { | |
@Override | |
public String lookup(String key) { | |
if (key.length() > 0) { | |
try { | |
String prop = System.getProperty(key); | |
if (prop != null) { | |
return prop; | |
} | |
return System.getenv(key); | |
} catch (final SecurityException scex) { | |
return null; | |
} | |
} | |
return null; | |
} | |
}; | |
StrSubstitutor strSubstitutor = new StrSubstitutor(variableResolver); | |
return strSubstitutor.replace(path); |
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.
@@ -1454,15 +1454,40 @@ public Preferences setGradleUserHome(String gradleUserHome) { | |||
} | |||
|
|||
public Preferences setFormatterUrl(String formatterUrl) { | |||
this.formatterUrl = ResourceUtils.expandPath(formatterUrl); | |||
this.formatterUrl = this.resolveWorkspaceFolder(ResourceUtils.expandPath(formatterUrl)); |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would just return filePath
if nothing exists on any of the root paths.
return this; | ||
} | ||
|
||
private String resolveWorkspaceFolder(String filePath) { | ||
final String workspaceFolderVariable = "${workspaceFolder}"; |
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.
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