Skip to content

Fix #15055 #15056

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

Closed
wants to merge 1 commit into from
Closed

Fix #15055 #15056

wants to merge 1 commit into from

Conversation

mbovel
Copy link
Member

@mbovel mbovel commented Apr 28, 2022

[test_windows_full]

@mbovel mbovel requested a review from som-snytt April 28, 2022 09:26
@mbovel
Copy link
Member Author

mbovel commented Apr 28, 2022

@som-snytt, would that make sense to use Path.getFileName here?

@mbovel mbovel force-pushed the mb/fix-file-name branch from 63c87cf to a1ce896 Compare April 28, 2022 09:38
@mbovel
Copy link
Member Author

mbovel commented Apr 28, 2022

would that make sense to use Path.getFileName here?

No, because using Path induces additional validation of the path that we do not want here:

java.nio.file.InvalidPathException: Illegal char <<> at index 0: <highlighting>

I forced-pushed a second attempt that just replaces / with File.separator.

@som-snytt
Copy link
Contributor

I'll take a look (when fully awake), but there's no reason for virtual paths to respect the syntax of some random other file system (such as Windows).

@som-snytt
Copy link
Contributor

I proposed converting higher up the food chain at SourceFile.virtual #15064

Alternatively, creating a VirtualFile could require supplying both path and name. Most usages are of the form <stuff> rather than fancy paths. That must be the intuition behind the constructor that takes only a name; but it would be more regular for that constructor to also split the incoming string. Maybe I will go ahead and do that. I'll compare current usages; probably anyone expecting to supply a path also supplied the content?

@som-snytt
Copy link
Contributor

som-snytt commented Apr 28, 2022

Also, I've been working under WSL for some time, and just revived my cygwin, so I'll try to test locally under windows. Worst case, actually try using windows tooling, shudder.

cygwin:

[?2004hsbt:scala3←[36m> ←[0m

I'm grateful for the person who wrote on SO to add -Dsbt.log.noformat=true. I always have to google it.

←[?1l←>←[?1000l←[?2004l←[?1h←=←[?2004hsbt:scala3>

Same with -no-colors. You can see why Scala 3's -color:never irks me at this point.

@som-snytt
Copy link
Contributor

would that make sense to use Path.getFileName here?

worth adding that InteractiveDriver did that before the recent tweak.

I also considered that VirtualFile should require user to supply both path and simple name if they are not the same. Then the strings could be taken as opaque instead of structured.

Copy link
Contributor

@som-snytt som-snytt left a comment

Choose a reason for hiding this comment

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

these classes are still an unfortunate weak spot, or "experimental". Since VirtualDirectory uses / it seems more robust to say, No, virtual means not tied to your local conventions. The alternative PR makes SourceFile convert the slashes.

@mbovel mbovel closed this Apr 28, 2022
@mbovel mbovel deleted the mb/fix-file-name branch April 28, 2022 19:48
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