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
+54 −13
Merged

FIX: Handle root path correctly #3354

Changes from 1 commit
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
Prev Previous commit
Next Next commit
Use string instead of view
@InversionSpaces
InversionSpaces committed Nov 30, 2023
commit b35d234c14b2739605073cdb4cd8cffccf8d4e12
25 changes: 10 additions & 15 deletions io/js/src/main/scala/fs2/io/file/Path.scala
View file
Original file line number Diff line number Diff line change
@@ -26,7 +26,6 @@ package file
import fs2.io.internal.facade

import scala.annotation.tailrec
import scala.collection.IndexedSeqView

final case class Path private[file] (override val toString: String) extends PathApi {

@@ -99,20 +98,16 @@ final case class Path private[file] (override val toString: String) extends Path
object Path extends PathCompanionApi {
private[file] val sep = facade.path.sep

private[file] def dropTrailingSep(path: String): String = {
/*
* 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
def drop(view: IndexedSeqView[Char]): IndexedSeqView[Char] =
// Drop separator only if there is something else left
if (view.endsWith(sep) && view.length > sep.length)
drop(view.dropRight(sep.length))
else view

drop(path.view).mkString
}
/*
* 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.
2 changes: 1 addition & 1 deletion io/shared/src/test/scala/fs2/io/file/PathSuite.scala
View file
Original file line number Diff line number Diff line change
@@ -52,7 +52,7 @@ class PathSuite extends Fs2IoSuite {
)
)
forAll((sepLength: Int, path: Path) =>
if (sepLength >= 0 && sepLength <= 100)
if (sepLength >= 0 && sepLength <= 100 && path.toString.nonEmpty)
assertEquals(
Path(path.toString + "/".repeat(sepLength)),
path