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

make os.root.baseName equal to s"${os.root/}" rather than throwing an exception #277

Closed
wants to merge 2 commits into from

Conversation

philwalk
Copy link
Contributor

@philwalk philwalk commented Jun 11, 2024

This addresses the problems described by #276
It provides a fix for VirtusLab/scala-cli#2954
It makes os.root.last the empty string rather than throwing LastOnEmptyPath.
Then, it defines os.root.baseName as s"$driveRoot/", rather than throwing an Exception.

@philwalk philwalk changed the title make os.root.baseName equal to os.root rather than the empty string make os.root.baseName equal to os.root rather than throwing an exception Jun 11, 2024
@philwalk philwalk changed the title make os.root.baseName equal to os.root rather than throwing an exception make os.root.baseName equal to s"${os.root/}" rather than throwing an exception Jun 14, 2024
@lihaoyi
Copy link
Member

lihaoyi commented Jun 25, 2024

TBH I'm not sure if this is the correct behavior for this. I think it's reasonable for it to fail and downstream to handle it, v.s. arbitrarily returning empty strings.

There are tons of cases where empty strings are not what you want either, and they have a tendency to propagate far beyond the point of creation before misbehaving in complicated ways. In comparison, the exception immediately tells you what's going on and you can remediate it at the failure site

@philwalk
Copy link
Contributor Author

philwalk commented Jun 25, 2024

TBH I'm not sure if this is the correct behavior for this. I think it's reasonable for it to fail and downstream to handle it, v.s. arbitrarily returning empty strings.

I don't disagree with your reasoning. A concern is that throwing of an exception makes it unsafe to call .baseName on any os.Path that might be === os.root. Empty string is a reasonable way to represent that a path has no segments, implying that it's os.root. This is similar to the way that dirname / returns / rather than an error message.

On the other hand, this change might cause problems with existing client code, so perhaps this needs to be handled the way you suggest, thanks for the feedback.

@philwalk philwalk closed this Jun 26, 2024
@philwalk philwalk deleted the root-script-mod branch June 26, 2024 21:07
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