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

Replace hardcoded uses of ":" by File.pathSeparator #5000

Closed
smarter opened this issue Aug 24, 2018 · 6 comments
Closed

Replace hardcoded uses of ":" by File.pathSeparator #5000

smarter opened this issue Aug 24, 2018 · 6 comments

Comments

@smarter
Copy link
Member

smarter commented Aug 24, 2018

":" is used in various places in the codebase, this is wrong (it doesn't work on Windows) and should be replaced by File.pathSeparator

@smarter smarter changed the title Replace hardcoded uses of ":" by Path.separator Replace hardcoded uses of ":" by File.pathSeparator Aug 24, 2018
@Blaisorblade
Copy link
Contributor

Blaisorblade commented Aug 24, 2018

Regarding Windows, I noticed we use /, is that OK? Shouldn't we use File.separator?

@sjrd
Copy link
Member

sjrd commented Aug 24, 2018

/ is usually OK on Windows in Java, because the abstractions will convert them. Except of course if you get a file name from the file system the look for /es in it. If you do that you should first normalize.

@Blaisorblade
Copy link
Contributor

Some progress in #5085 by @martijnhoekstra, but they suggest we shouldn’t be closing this yet.

@martijnhoekstra
Copy link
Contributor

@Blaisorblade I would suggest closing this for now -- I can't promise there are absolutely no hardcoded strings containing : remaining, though I reviewed all strings ":" and chars ':', there may still be "foo:bar"'s that are harder to find -- but this issue isn't actionable anymore I think

@martijnhoekstra
Copy link
Contributor

martijnhoekstra commented Sep 15, 2018

in scala, the platform-dependency issues tend to be

  • Classpath separators (like here)
  • checking for path separators '/' (though using '/' is fine)
  • line endings. the difference between \r\n and \n represents a semantic difference if the line end is part of a multiline string, and println will emit \r\n on windows. Adding insult to injury, if windows users have git autocrlf set (out of dubious recommendations of compatibility) they may have crlf in their local repos where there is no such \r present in the original source.

Ironically, many issues occur when one part of some system attempts to do platform compatibility for unneeded reasons (java println, git autoclrf, auto-conversion of /), and other parts don't, and then suddenly you have situations where you're checking for \r\n on windows while there is a \n which is working just fine, leading to errors or test failures.

@Blaisorblade
Copy link
Contributor

Closing on @martijnhoekstra’s recommendation.

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

No branches or pull requests

5 participants