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.

Sign up for GitHub

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

Jump to bottom

fix for issue-201 #202

Merged
merged 11 commits into from
Sep 12, 2023
Merged

fix for issue-201 #202

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
abc911e
fix for issue-201
philwalk Aug 28, 2023
d20e417
updated PR based on suggestions and discussion
philwalk Aug 29, 2023
6c04015
fix scalafmt errors
philwalk Aug 31, 2023
0707d4c
all os/test/src/PathTests.sc enabled, new test added, allow rootRelat…
philwalk Sep 3, 2023
1c37739
removed incorrect comment
philwalk Sep 3, 2023
5bee44c
changes suggested by @lihaoyi
philwalk Sep 9, 2023
1570df4
fix scalafmt errors, supplement PathTests.scala near line 25 test
philwalk Sep 10, 2023
fc78871
fix test "Paths.get(platformPrefix) sameFile as pwd"
philwalk Sep 10, 2023
430951e
renamed `platformPrefix` and `rootRelative`, fixed java8 failing test
philwalk Sep 10, 2023
93ef3aa
fix java8 trailing slash test error
philwalk Sep 10, 2023
c1b0cab
scalafmt fix
philwalk Sep 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ target/
.project
.cache
.sbtserver
.scala-build/
.bsp/
project/.sbtserver
tags
nohup.out
Expand Down
47 changes: 39 additions & 8 deletions os/src/Path.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,10 +237,12 @@ sealed trait FilePath extends BasePath {
def toNIO: java.nio.file.Path
def resolveFrom(base: os.Path): os.Path
}

object FilePath {
def apply[T: PathConvertible](f0: T) = {
val f = implicitly[PathConvertible[T]].apply(f0)
if (f.isAbsolute) Path(f0)
def f = implicitly[PathConvertible[T]].apply(f0)
// if Windows root-relative path, convert it to an absolute path
if (Path.rootRelative(f0) || f.isAbsolute) Path(f0)
else {
val r = RelPath(f0)
if (r.ups == 0) r.asSubPath
Expand Down Expand Up @@ -297,7 +299,7 @@ object RelPath {
def apply[T: PathConvertible](f0: T): RelPath = {
val f = implicitly[PathConvertible[T]].apply(f0)

require(!f.isAbsolute, s"$f is not a relative path")
require(!f.isAbsolute && !Path.rootRelative(f0), s"$f is not a relative path")

val segments = BasePath.chunkify(f.normalize())
val (ups, rest) = segments.partition(_ == "..")
Expand Down Expand Up @@ -398,13 +400,16 @@ object Path {

def apply[T: PathConvertible](f: T, base: Path): Path = apply(FilePath(f), base)
def apply[T: PathConvertible](f0: T): Path = {
val f = implicitly[PathConvertible[T]].apply(f0)
// drive letter prefix is empty unless running in Windows.
val f = if (rootRelative(f0)) {
Paths.get(s"$platformPrefix$f0")
} else {
implicitly[PathConvertible[T]].apply(f0)
}
if (f.iterator.asScala.count(_.startsWith("..")) > f.getNameCount / 2) {
throw PathError.AbsolutePathOutsideRoot
}

val normalized = f.normalize()
new Path(normalized)
new Path(f.normalize())
}

implicit val pathOrdering: Ordering[Path] = new Ordering[Path] {
Expand Down Expand Up @@ -436,6 +441,32 @@ object Path {
}
}

/**
* @return true if Windows OS and path begins with slash or backslash.
* Examples:
* rootRelative("/Users") // true in `Windows`, false elsewhere.
* rootRelative("\\Users") // true in `Windows`, false elsewhere.
* rootRelative("C:/Users") // false always
*/
def rootRelative[T: PathConvertible](f0: T): Boolean = {
philwalk marked this conversation as resolved.
Show resolved Hide resolved
if (platformPrefix.isEmpty) {
false // non-Windows os
} else {
f0.toString.take(1) match {
case "\\" | "/" => true
case _ => false
}
}
}

/**
* @return current working drive if Windows, empty string elsewhere.
* Paths.get(platformPrefix) == current working directory on all platforms.
*/
lazy val platformPrefix: String = Paths.get(".").toAbsolutePath.getRoot.toString match {
Copy link
Member

Choose a reason for hiding this comment

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

Overall I'm fine with this PR, but I'm still struggling with the term platformPrefix. Hhow about rootPrefix or maybe also something that has the "drive" in it?

Copy link
Contributor Author

@philwalk philwalk Sep 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I like driveRoot instead of platformPrefix
and driveRelative instead of rootRelative.

case "/" => "" // implies a non-Windows platform
case s => s.take(2) // Windows current working drive (e.g., "C:")
}
}

trait ReadablePath {
Expand All @@ -452,7 +483,7 @@ class Path private[os] (val wrapped: java.nio.file.Path)
def toSource: SeekableSource =
new SeekableSource.ChannelSource(java.nio.file.Files.newByteChannel(wrapped))

require(wrapped.isAbsolute, s"$wrapped is not an absolute path")
require(wrapped.isAbsolute || Path.rootRelative(wrapped), s"$wrapped is not an absolute path")
def segments: Iterator[String] = wrapped.iterator().asScala.map(_.toString)
def getSegment(i: Int): String = wrapped.getName(i).toString
def segmentCount = wrapped.getNameCount
Expand Down
Loading
Loading