Closed kiendang closed 3 months ago
Looks good to me. @kiendang if you could update the test to use intercept I can merge this and close the bounty
@lefou @lihaoyi I did use intercept
earlier but there were a few issues with using intercept
in the os.move.matching
case. It throws a java.lang.IllegalArgumentException
(rather than a FileAlreadyExistsException
) which AbsolutePathOutsideRoot
extends, so the test wouldn't fail without the fix. Plus there seems to be compilation error with using os.move.matching
pattern matching syntax inside a intercept
block. And also the purpose is to test that the os.move
and os.move.matching
cases don't throw AbsolutePathOutsideRoot
rather that they throws a specific exception so went with the current approach instead.
More on the error with using pattern matching syntax inside a intercept block, this would result in an error
intercept[...] {
os.list(os.root("/", fileSystem)).collect(os.move.matching { case p / "test" => p })
}
while this wouldn't
intercept[...] {
List(os.root("/", fileSystem)).collect(os.move.matching { case p => p })
}
More on the error with using pattern matching syntax inside a intercept block
Which error do you mean specifically? The FileAlreadyExistsException
?
This test works for me with Scala 2.13:
test("moveMatchingToRootDirectoryShouldFail") {
withCustomFs { fileSystem =>
intercept[FileAlreadyExistsException] {
os.list(os.root("/", fileSystem)).collect(os.move.matching { case p / "test" => p })
}
}
}
But with Scala 2.12.17, I see the same compile error as the CI:
1 targets failed
os.jvm[2.12.17].test.compile scala.reflect.internal.FatalError:
unexpected UnApply os.`package`./.unapply({
val $temp$macro$62 = <unapply-selector>;
$log$macro$59(utest.TestValue("<unapply-selector>", "os.Path", $temp$macro$62));
$temp$macro$62
}) <unapply> ((p @ _), "test")
while compiling: /home/lefou/work/opensource/os-lib/os/test/src-jvm/PathTestsCustomFilesystem.scala
during phase: globalPhase=typer, enteringPhase=namer
library version: version 2.12.17
compiler version: version 2.12.17
reconstructed args: -bootclasspath /home/lefou/.cache/coursier/v1/https/repo1.maven.org/maven2/org/scala-lang/scala-library/2.12.17/scala-library-2.12.17.jar -classpath /home/lefou/.cache/coursier/v1/https/repo1.maven.org/maven2/com/lihaoyi/utest_2.12/0.8.3/utest_2.12-0.8.3.jar:/home/lefou/.cache/coursier/v1/https/repo1.maven.org/maven2/com/lihaoyi/sourcecode_2.12/0.4.0/sourcecode_2.12-0.4.0.jar:/home/lefou/.cache/coursier/v1/https/repo1.maven.org/maven2/com/lihaoyi/geny_2.12/1.1.0/geny_2.12-1.1.0.jar:/home/lefou/.cache/coursier/v1/https/repo1.maven.org/maven2/org/scala-sbt/test-interface/1.0/test-interface-1.0.jar:/home/lefou/.cache/coursier/v1/https/repo1.maven.org/maven2/org/portable-scala/portable-scala-reflect_2.12/1.1.2/portable-scala-reflect_2.12-1.1.2.jar:/home/lefou/.cache/coursier/v1/https/repo1.maven.org/maven2/org/scala-lang/scala-reflect/2.12.17/scala-reflect-2.12.17.jar:/home/lefou/work/opensource/os-lib/os/compile-resources:/home/lefou/work/opensource/os-lib/out/os/jvm/2.12.17/compile.dest/classes:/home/lefou/work/opensource/os-lib/os/test/compile-resources:/home/lefou/work/opensource/os-lib/out/os/jvm/2.12.17/test/compile.dest/classes -Xplugin:/home/lefou/.cache/coursier/v1/https/repo1.maven.org/maven2/com/lihaoyi/acyclic_2.12.17/0.3.12/acyclic_2.12.17-0.3.12.jar -P:acyclic:force
last tree to typer: UnApply
tree position: line 213 of /home/lefou/work/opensource/os-lib/os/test/src-jvm/PathTestsCustomFilesystem.scala
symbol: null
call site: method applyOrElse in package os
== Source file context for tree position ==
210 withCustomFs { fileSystem =>
211 // This should fail. Just test that it doesn't throw PathError.AbsolutePathOutsideRoot.
212 intercept[FileAlreadyExistsException] {
213 os.list(os.root("/", fileSystem)).collect(os.move.matching { case p / "test" => p })
214 }
215 }
216 }
scala.reflect.internal.Reporting.abort(Reporting.scala:69)
scala.reflect.internal.Reporting.abort$(Reporting.scala:65)
scala.reflect.internal.SymbolTable.abort(SymbolTable.scala:28)
scala.tools.nsc.typechecker.Typers$Typer.typed1(Typers.scala:5745)
scala.tools.nsc.typechecker.Typers$Typer.typed(Typers.scala:5785)
I tested on Scala 3.3.1 and jdk 17. For me it also doesn't throw FileAlreadyExistsException
but IllegalArgumentException
/ is inside /
.
Also yes I got a similar compilation error.
Looks good to me. Thanks @kiendang! Can send me an email at haoyi.sg@gmail.com with your bank details for the bounty
Fix an error where
os.copy(createFolders = true, mergeFolders = true)
a directory to root throw aPathError.AbsolutePathOutsideRoot
due to calling/up
on root.The same issue (calling
/up
on root) is seen inos.move
,os.copy.matching
andos.move.matching
so this fixes those as well. In case ofos.move
andos.move.matching
moving a directory to root should throw an error anyway but at least now it throws the correct underlying error instead ofAbsolutePathOutsideRoot
.Fix https://github.com/com-lihaoyi/os-lib/issues/167
Pull request: https://github.com/com-lihaoyi/os-lib/pull/267