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: Handle root path correctly #3354

Merged
merged 8 commits into from
Dec 6, 2023
Merged

FIX: Handle root path correctly #3354

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
cd297d2
Drop trailing spaces, add tests
InversionSpaces Nov 30, 2023
e0d836f
Add comments
InversionSpaces Nov 30, 2023
b35d234
Use string instead of view
InversionSpaces Nov 30, 2023
87bee7d
Use path.parse + path.format
InversionSpaces Dec 5, 2023
5c5e5ff
Fix generators in tests
InversionSpaces Dec 5, 2023
f0a4373
Refactor comment
InversionSpaces Dec 5, 2023
6b2d8a3
Remove unused import
InversionSpaces Dec 5, 2023
0e2495d
Fix tests
InversionSpaces Dec 6, 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
20 changes: 16 additions & 4 deletions io/js/src/main/scala/fs2/io/file/Path.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,9 +98,21 @@ final case class Path private[file] (override val toString: String) extends Path
object Path extends PathCompanionApi {
private[file] val sep = facade.path.sep

/*
* NOTE: It seems like scala js does not rewrite this to a loop?
* Call stack limit is reached if there are too many separators.
*/
@tailrec
private[file] def dropTrailingSep(path: String): String =
// Drop separator only if there is something else left
if (path.endsWith(sep) && path.length > sep.length)
Copy link
Member

Choose a reason for hiding this comment

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

I don't like to Officially:tm: support Windows 🤮 but its path can look like C:\ which I think would get re-written to C:. I have no idea if that's good or bad 😅

Copy link
Contributor

@BalmungSan BalmungSan Nov 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Doing some tests on Powershell

PS C:\Users\PC> cd c:
PS C:\Users\PC> cd C:
PS C:\Users\PC> cd C:\
PS C:\>

Thus, it seems they are not equivalent.

IMHO, it is best to avoid this specific Path modification, also it may be good to add a unit test verifying 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.

Used node API exclusively

dropTrailingSep(path.dropRight(sep.length))
else path
Copy link
Member

Choose a reason for hiding this comment

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

It does actually 🤔 Here's what it's compiled to:

$c_Lfs2_io_file_Path$.prototype.dropTrailingSep__T__T = (function(path) {
  while (true) {
    if ($f_T__endsWith__T__Z($n(path), this.Lfs2_io_file_Path$__f_sep)) {
      var this$1 = $n(path);
      var this$2 = $n(this.Lfs2_io_file_Path$__f_sep);
      var $$x2 = (this$1.length > this$2.length)
    } else {
      var $$x2 = false
    };
    if ($$x2) {
      var $$x1 = $m_sc_StringOps$();
      var x = path;
      var this$4 = $n(this.Lfs2_io_file_Path$__f_sep);
      path = $n($$x1).dropRight$extension__T__I__T(x, this$4.length)
    } else {
      break
    }
  };
  return path
});

Copy link
Member

Choose a reason for hiding this comment

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

This passes for me. How were you getting the failure?

assertEquals(Path("/".repeat(Int.MaxValue/8)), Path("/"))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Strange, I wrote this comment when I used IndexedSeqView instead of String.


/** This method drops trailing separators to match
* `java.nio.file.Path.get` behaviour.
* But root ("/") is untouched.
*/
def apply(path: String): Path =
if (path.endsWith(sep))
new Path(path.dropRight(sep.length))
else
new Path(path)
new Path(dropTrailingSep(path))
}
14 changes: 14 additions & 0 deletions io/shared/src/test/scala/fs2/io/file/PathSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,20 @@ class PathSuite extends Fs2IoSuite {
test("construction") {
assertEquals(Path("foo/bar"), Path("foo") / "bar")
assertEquals(Path("/foo/bar"), Path("/foo") / "bar")
forAll((sepLength: Int) =>
if (sepLength >= 0 && sepLength <= 100)
Copy link
Member

Choose a reason for hiding this comment

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

These if guards are probably skipping most/all of the generated values. Instead you should do something like this.

Suggested change
forAll((sepLength: Int) =>
if (sepLength >= 0 && sepLength <= 100)
forAll(Gen.choose(0, 100))((sepLength: Int) =>

assertEquals(
Path("/".repeat(sepLength)).toString,
"/"
)
)
forAll((sepLength: Int, path: Path) =>
if (sepLength >= 0 && sepLength <= 100 && path.toString.nonEmpty)
assertEquals(
Path(path.toString + "/".repeat(sepLength)),
path
)
)
}

test("normalize") {
Expand Down