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
bugfix: Fix running when spaces are involved on windows #4815
Conversation
.replace("\\\"", "") | ||
.replace("\"\\", "\\") | ||
// when testing on Windows Program Files is problematic when splitting into list | ||
.replace("Program Files", "ProgramFiles") |
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 am so very sorry about this :O
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.
Woof
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.
The other option would be to find a process runner that takes in a whole string, but that seems even more work :/
Previously, we would not escape Java path, classpath or java properties, which meant that whenever spaces were involved things would not work on Windows (possibly on some other systems too). Now, we properly escape all those things. I tested both by hand and added a test case, which proved to be a pain to write :/ Fixes scalameta#4801
3908c2b
to
468bb97
Compare
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 mean, we have tests, so I feel ok about this, but I wish there was an easy way to give any sort of path to a function and have it make it safe for windows.
Either way with the tests and your manual testing, LGTM.
metals/src/main/scala/scala/meta/internal/metals/debug/ExtendedScalaMainClass.scala
Outdated
Show resolved
Hide resolved
.replace("\\\"", "") | ||
.replace("\"\\", "\\") | ||
// when testing on Windows Program Files is problematic when splitting into list | ||
.replace("Program Files", "ProgramFiles") |
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.
Woof
…dScalaMainClass.scala Co-authored-by: Chris Kipp <ckipp@pm.me>
It's really fantastic to see Windows issues being handled so quickly 👏 !! |
Previously, we would not escape Java path, classpath or java properties, which meant that whenever spaces were involved things would not work on Windows (possibly on some other systems too). Now, we properly escape all those things.
I tested both by hand and added a test case, which proved to be a pain to write :/
Fixes #4801