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

java.project.referencedLibraries should resolve paths leading with ~ #1739

Merged
merged 1 commit into from
Apr 23, 2021

Conversation

snjeza
Copy link
Contributor

@snjeza snjeza commented Apr 22, 2021

Fixes #1735

  • replaces paths leading with ~ with the user.home system property
  • replaces ${} with their matching values from the environment variables and system properties
  • resolves java.project.referencedLibraries, java.import.gradle.home, java.import.gradle.java.home, java.import.gradle.user.home, java.configuration.maven.userSettings, java.configuration.maven.globalSettings, java.project.outputPath, java.project.sourcePaths, java.format.settings.url, java.settings.url, java.configuration.runtimes

Signed-off-by: Snjezana Peco [email protected]

Copy link
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.

Nice. So we had ResourceUtils#expandPath(..) all along but just working on the maven user/global settings. I think this is a nice improvement. Technically users can have file/folders like ${foo} (at least on Linux). Does this still allow such a file/folder to be used assuming the user hasn't defined ${foo} in system properties/env ?

@snjeza
Copy link
Contributor Author

snjeza commented Apr 22, 2021

Technically users can have file/folders like ${foo} (at least on Linux). Does this still allow such a file/folder to be used assuming the user hasn't defined ${foo} in system properties/env ?

see 199653e#diff-8699e13c637b9d38b7ec8c77ffc66ecd9443e94dde8d57347fdea1115560631fR266

@snjeza
Copy link
Contributor Author

snjeza commented Apr 22, 2021

So we had ResourceUtils#expandPath(..) all along but just working on the maven user/global settings

Before, ResourceUtils#expandPath(..) was just replacing leading ~.

@rgrunber
Copy link
Contributor

Overall, looks good to me. @fbricon let me know your thoughts. Only thing I can think of that really isn't our issue, is if a user of some client becomes confused by the fact that the client itself doesn't support such a feature. Then again, we already supported '~' . We didn't really document this on the server side, so it's up to clients to decide whether they want to mention this ability.

@rgrunber
Copy link
Contributor

rgrunber commented Apr 23, 2021

test this please
(didn't see the failure locally)

@rgrunber
Copy link
Contributor

I would modify the commit to include the things in your main PR comment. From what I can see, the commit only has the title, but no reference to the issue, or the additional enhancements listed in #1739 (comment) .

@rgrunber
Copy link
Contributor

test this please
(different failure, still seems unrelated)

@snjeza
Copy link
Contributor Author

snjeza commented Apr 23, 2021

I would modify the commit to include the things in your main PR comment. From what I can see, the commit only has the title, but no reference to the issue, or the additional enhancements listed in #1739 (comment) .

Fixed.

@snjeza
Copy link
Contributor Author

snjeza commented Apr 23, 2021

test this please

1 similar comment
@rgrunber
Copy link
Contributor

test this please

@rgrunber
Copy link
Contributor

rgrunber commented Apr 23, 2021

@snjeza can you just wrap each line of the commit message body to <= 72 characters, and then I think we can merge.

- replaces paths leading with ~ with the user.home system property
- replaces ${} with their matching values from the environment variables
  and system properties
- resolves java.project.referencedLibraries, java.import.gradle.home,
  java.import.gradle.java.home, java.import.gradle.user.home,
  java.configuration.maven.userSettings,
  java.configuration.maven.globalSettings, java.project.outputPath,
  java.project.sourcePaths, java.format.settings.url,
  java.settings.url, java.configuration.runtimes

Signed-off-by: Snjezana Peco <[email protected]>
@snjeza
Copy link
Contributor Author

snjeza commented Apr 23, 2021

@snjeza can you just wrap each line of the commit message body to <= 72 characters, and then I think we can merge.

Done.

@rgrunber rgrunber merged commit 96e8298 into eclipse-jdtls:master Apr 23, 2021
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.

java.project.referencedLibraries should resolve paths leading with ~
3 participants