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

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 5 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
53 changes: 44 additions & 9 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 semi-absolute 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,12 +400,18 @@ 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)
if (f.iterator.asScala.count(_.startsWith("..")) > f.getNameCount / 2) {
throw PathError.AbsolutePathOutsideRoot
// drive letter prefix is empty unless running in Windows.
val normalized = {
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
}
Copy link
Member

Choose a reason for hiding this comment

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

We can re-arrange this to move the throw outside of the val normalized? All these are method-local vals anyway, so there's no visibility/privacy concerns, and it would be nice to remove a layer of nesting

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

f.normalize()
}

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

Expand Down Expand Up @@ -436,6 +444,33 @@ 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 = rootRelative(f0.toString)

def rootRelative(s: String): Boolean = {
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this overload, given that the implicit object StringConvertible extends PathConvertible[String] already exists? If not we should remove it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Combined the two:

  def rootRelative[T: PathConvertible](f0: T): Boolean = {
    if (platformPrefix.isEmpty) {
      false // non-Windows os
    } else {
      f0.toString.take(1) match {
        case "\\" | "/" => true
        case _ => false
      }
    }
  }

if (platformPrefix.isEmpty) {
false // non-Windows os
} else {
s.toString.take(1) match {
case "\\" | "/" => true
case _ => false
}
}
}

/**
* @return current working drive if Windows, empty string elsewhere.
*/
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 +487,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
84 changes: 58 additions & 26 deletions os/test/src/PathTests.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,17 +3,20 @@ package test.os
import java.nio.file.Paths

import os._
import os.Path.{platformPrefix}
import utest.{assert => _, _}
import java.net.URI
object PathTests extends TestSuite {
def enableTest = true
Copy link
Member

Choose a reason for hiding this comment

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

If enableTest is true, we should be able to remove it and remove all the conditionals guarding the test cases

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll remove them.

val tests = Tests {
test("Basic") {
val base = rel / "src" / "main" / "scala"
val subBase = sub / "src" / "main" / "scala"
test("Transformers") {
if (Unix()) {
// os.Path to java.nio.file.Path
assert((root / "omg").wrapped == Paths.get("/omg"))
if (enableTest) {
// compare os.Path to java.nio.file.Path
assert((root / "omg").norm == Paths.get("/omg").norm)
assert(sameFile((root / "omg").wrapped, Paths.get("/omg")))

// java.nio.file.Path to os.Path
assert(root / "omg" == Path(Paths.get("/omg")))
Expand All @@ -34,7 +37,7 @@ object PathTests extends TestSuite {
intercept[IllegalArgumentException](Path(ldapUri))

// os.Path to String
assert((root / "omg").toString == "/omg")
assert((root / "omg").posix == s"$platformPrefix/omg")
assert((rel / "omg").toString == "omg")
assert((sub / "omg").toString == "omg")
assert((up / "omg").toString == "../omg")
Expand Down Expand Up @@ -84,7 +87,7 @@ object PathTests extends TestSuite {
test("RelPath") {
test("Constructors") {
test("Symbol") {
if (Unix()) {
if (enableTest) {
val rel1 = base / "ammonite"
assert(
rel1.segments == Seq("src", "main", "scala", "ammonite"),
Expand All @@ -93,7 +96,7 @@ object PathTests extends TestSuite {
}
}
test("String") {
if (Unix()) {
if (enableTest) {
val rel1 = base / "Path.scala"
assert(
rel1.segments == Seq("src", "main", "scala", "Path.scala"),
Expand All @@ -107,25 +110,25 @@ object PathTests extends TestSuite {
rel1.toString == "src/main/scala/sub1/sub2"
)
test("ArrayString") {
if (Unix()) {
if (enableTest) {
val arr = Array("sub1", "sub2")
check(base / arr)
}
}
test("ArraySymbol") {
if (Unix()) {
if (enableTest) {
val arr = Array("sub1", "sub2")
check(base / arr)
}
}
test("SeqString") {
if (Unix()) check(base / Seq("sub1", "sub2"))
if (enableTest) check(base / Seq("sub1", "sub2"))
}
test("SeqSymbol") {
if (Unix()) check(base / Seq("sub1", "sub2"))
if (enableTest) check(base / Seq("sub1", "sub2"))
}
test("SeqSeqSeqSymbol") {
if (Unix()) {
if (enableTest) {
check(
base / Seq(Seq(Seq("sub1"), Seq()), Seq(Seq("sub2")), Seq())
)
Expand All @@ -148,7 +151,7 @@ object PathTests extends TestSuite {
test("SubPath") {
test("Constructors") {
test("Symbol") {
if (Unix()) {
if (enableTest) {
val rel1 = subBase / "ammonite"
assert(
rel1.segments == Seq("src", "main", "scala", "ammonite"),
Expand All @@ -157,7 +160,7 @@ object PathTests extends TestSuite {
}
}
test("String") {
if (Unix()) {
if (enableTest) {
val rel1 = subBase / "Path.scala"
assert(
rel1.segments == Seq("src", "main", "scala", "Path.scala"),
Expand All @@ -171,25 +174,25 @@ object PathTests extends TestSuite {
rel1.toString == "src/main/scala/sub1/sub2"
)
test("ArrayString") {
if (Unix()) {
if (enableTest) {
val arr = Array("sub1", "sub2")
check(subBase / arr)
}
}
test("ArraySymbol") {
if (Unix()) {
if (enableTest) {
val arr = Array("sub1", "sub2")
check(subBase / arr)
}
}
test("SeqString") {
if (Unix()) check(subBase / Seq("sub1", "sub2"))
if (enableTest) check(subBase / Seq("sub1", "sub2"))
}
test("SeqSymbol") {
if (Unix()) check(subBase / Seq("sub1", "sub2"))
if (enableTest) check(subBase / Seq("sub1", "sub2"))
}
test("SeqSeqSeqSymbol") {
if (Unix()) {
if (enableTest) {
check(
subBase / Seq(Seq(Seq("sub1"), Seq()), Seq(Seq("sub2")), Seq())
)
Expand All @@ -210,8 +213,8 @@ object PathTests extends TestSuite {
val d = pwd
val abs = d / base
test("Constructor") {
if (Unix()) assert(
abs.toString.drop(d.toString.length) == "/src/main/scala",
assert(
abs.posix.drop(d.toString.length) == "/src/main/scala",
abs.toString.length > d.toString.length
)
}
Expand Down Expand Up @@ -307,7 +310,7 @@ object PathTests extends TestSuite {
intercept[PathError.InvalidSegment](rel / "src" / "..")
}
test("CannotRelativizeAbsAndRel") {
if (Unix()) {
if (enableTest) {
val abs = pwd
val rel = os.rel / "omg" / "wtf"
compileError("""
Expand All @@ -319,7 +322,7 @@ object PathTests extends TestSuite {
}
}
test("InvalidCasts") {
if (Unix()) {
if (enableTest) {
intercept[IllegalArgumentException](Path("omg/cow"))
intercept[IllegalArgumentException](RelPath("/omg/cow"))
}
Expand Down Expand Up @@ -366,7 +369,7 @@ object PathTests extends TestSuite {
}
test("construction") {
test("success") {
if (Unix()) {
if (enableTest) {
val relStr = "hello/cow/world/.."
val absStr = "/hello/world"

Expand All @@ -390,7 +393,7 @@ object PathTests extends TestSuite {
}
}
test("basepath") {
if (Unix()) {
if (enableTest) {
val relStr = "hello/cow/world/.."
val absStr = "/hello/world"
assert(
Expand All @@ -400,7 +403,7 @@ object PathTests extends TestSuite {
}
}
test("based") {
if (Unix()) {
if (enableTest) {
val relStr = "hello/cow/world/.."
val absStr = "/hello/world"
val basePath: FilePath = FilePath(relStr)
Expand All @@ -411,7 +414,7 @@ object PathTests extends TestSuite {
}
}
test("failure") {
if (Unix()) {
if (enableTest) {
val relStr = "hello/.."
intercept[java.lang.IllegalArgumentException] {
Path(relStr)
Expand All @@ -436,5 +439,34 @@ object PathTests extends TestSuite {
assert(result1 == expected)
assert(result2 == expected)
}
test("issue201") {
Path("/omg") // rootRelative path does not throw exception.
}
}
// compare absolute paths
def sameFile(a: java.nio.file.Path, b: java.nio.file.Path): Boolean = {
a.toAbsolutePath == b.toAbsolutePath
}
def sameFile(a: os.Path, b: java.nio.file.Path): Boolean = {
sameFile(a.wrapped, b)
}
def sameFile(a: Path, b: Path): Boolean = {
sameFile(a.wrapped, b.wrapped)
}
implicit class ExtendString(s: String) {
def posix: String = s.replace('\\', '/')
def norm: String = if (s.startsWith(platformPrefix)) {
s.posix // already has platformPrefix
} else {
s"$platformPrefix${s.posix}"
}
}
implicit class ExtendOsPath(p: os.Path) {
def posix: String = p.toNIO.toString.posix
def norm: String = p.toString.norm
}
implicit class ExtendPath(p: java.nio.file.Path) {
def posix: String = p.toString.posix
def norm: String = p.toString.norm
Copy link
Member

Choose a reason for hiding this comment

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

Let's use normal helper methods here, rather than extension methods, unless we have good reason to do otherwise.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done (will push changes soon)

}
}