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

Allow for relative paths in java home config #864

Merged
merged 3 commits into from
Feb 1, 2022

Conversation

dlobsinger
Copy link
Contributor

This change is an implementation to allow for the path provided in metals.javaHome to be a relative path from the root directory of the workspace.

The current behaviour when calling getJavaHome is that the path must be an absolute path or is resolved to a path relative to the system root /. If a user has a JDK located in their project directory, for example by using a nix profile to install a JDK in the same directory, it would be convenient to allow for the Metals configuration to use a path relative to the workspace.

@tgodzik tgodzik requested a review from kpodsiad January 31, 2022 15:16
Copy link
Member

@kpodsiad kpodsiad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your another contribution ;) I think we could simplify it a little bit, that immediate execution part wasn't obvious for me :c

src/extension.ts Outdated
Comment on lines 125 to 134
const javaHomeConfig = ((jhPath) =>
jhPath?.trim() && !path.isAbsolute(jhPath)
? path.resolve(
...[workspace.workspaceFolders?.[0]?.uri.fsPath, jhPath].filter(
(s): s is string => !!s
)
)
: jhPath)(
workspace.getConfiguration("metals").get<string>("javaHome")
);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking about little bit more verbose

export function getJavaHomeFromConfig(): string | undefined {
  const javaHomePath = workspace
    .getConfiguration("metals")
    .get<string>("javaHome");
  if (javaHomePath?.trim() && !path.isAbsolute(javaHomePath)) {
    const pathSegments = [
      workspace.workspaceFolders?.[0]?.uri.fsPath,
      javaHomePath,
    ].filter((s): s is string => s != null);
    return path.resolve(...pathSegments);
  } else {
    return javaHomePath;
  }
}

It's not as concise as what you're proposing but I find it easier to read ;)

Also, extension.ts is an enormous file, moving this function to e.g. util.ts might be a good idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point about moving this into util.ts - I wasn't sure whether it would be better to have as little impact in the scope of the code, or to have a more verbose implementation. I'll make those changes.

Copy link
Member

@kpodsiad kpodsiad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@kpodsiad kpodsiad merged commit b9be076 into scalameta:main Feb 1, 2022
@dlobsinger dlobsinger deleted the resolve-java-home-path branch February 1, 2022 14:28
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.

2 participants