Skip to content

Add workspace folder variable resolution for settings url#3442

Open
04kash wants to merge 1 commit intoeclipse-jdtls:mainfrom
04kash:resolve-workspaceFolder-in-settings-url
Open

Add workspace folder variable resolution for settings url#3442
04kash wants to merge 1 commit intoeclipse-jdtls:mainfrom
04kash:resolve-workspaceFolder-in-settings-url

Conversation

@04kash
Copy link
Copy Markdown

@04kash 04kash commented May 7, 2025

This PR adds support for resolving ${workspaceFolder} in the following settings:

  • java.format.settings.url
  • java.settings.url

If ${workspaceFolder} is present in the provided path, it will be replaced with the absolute path of the workspace folder (retrieved from getRootPaths()). The resolution logic checks all available workspace folders and uses the first matching path that exists.

Fixes: #2529

Signed-off-by: Kashish Mittal <kmittal@redhat.com>
@rgrunber rgrunber self-requested a review May 12, 2025 16:40
Copy link
Copy Markdown
Contributor

@rgrunber rgrunber left a 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 :

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);
. I think I liked this approach because it allows for many different variables to be defined and ultimately the client should know best since it sends the settings to begin with.

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));
Copy link
Copy Markdown
Contributor

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());
Copy link
Copy Markdown
Contributor

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.

}

private String resolveWorkspaceFolder(String filePath) {
final String workspaceFolderVariable = "${workspaceFolder}";
Copy link
Copy Markdown
Contributor

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.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add support for vscode path variables for settings url settings

2 participants