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.

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

ioJS: handle NoSuchFileException in isDirectory, isFile and isSymbolicLink #2727

Merged
merged 3 commits into from
Nov 30, 2021

Conversation

yurique
Copy link
Contributor

@yurique yurique commented Nov 29, 2021

In nodejs, calling stat throws an exception if the path does not exist, thus making isDirectory, isFile and isSymbolicLink fail as well. In jvm, on the other hand, the underlying calls do not throw and return false when the path does not exist.

This is an attempt to make the behaviour of these functions more aligned across platforms.


There's a couple of TODOs in the tests, not sure how important these are:

  • not sure how to easily create a symlink for tests
  • not sure how to create a "hidden" file for tests

@armanbilge
Copy link
Member

Thanks for the PR and improving the test coverage!

On the topic of tests, I think you can use this method to create a symlink (actually, I don't think that method is tested right now!):

override def createSymbolicLink(
link: Path,
target: Path,
permissions: Option[Permissions]
): F[Unit] =

Note sure how to easily create hidden files cross-OS (i.e., unix and Windows).

@yurique
Copy link
Contributor Author

yurique commented Nov 30, 2021

Added a true-case test for isSymbolicLink, which was pretty straight forward.

isHidden was more interesting.

First, the regex that was used (raw"/(^|\/)\.[^\/\.]/g".r) was no longer working at all. I believe it's due to the changes in Scala.js 1.7.0: https://www.scala-js.org/news/2021/08/04/announcing-scalajs-1.7.0/

This release fixes a number of bugs. In particular, regular expressions, available through java.util.regex.Pattern or Scala’s Regex and .r method, now behave in the same way as on the JVM. This change has compatibility implications, which we discuss below.

Confirmed this in ioJS/console (same in ioJVM/console):

The previous regex:

scala> val HiddenPattern = raw"/(^|\/)\.[^\/\.]/g".r
val HiddenPattern: scala.util.matching.Regex = /(^|\/)\.[^\/\.]/g

scala> def test(s: String) = HiddenPattern.findFirstIn(s).isDefined
def test(s: String): Boolean

scala> test("/var/folders/13/gsqmg365255bnrlnmn7m6pfr0000gn/T/GlfN5L/.should-be-hidden/not-hidden")
val res0: Boolean = false

Removing the g modifier:

scala> val HiddenPattern = raw"/(^|\/)\.[^\/\.]/".r
val HiddenPattern: scala.util.matching.Regex = /(^|\/)\.[^\/\.]/

scala> def test(s: String) = HiddenPattern.findFirstIn(s).isDefined
def test(s: String): Boolean

scala> test("/var/folders/13/gsqmg365255bnrlnmn7m6pfr0000gn/T/GlfN5L/.should-be-hidden/not-hidden")
val res1: Boolean = false

Finally, without /s it worked:

scala> val HiddenPattern = raw"(^|\/)\.[^\/\.]".r
val HiddenPattern: scala.util.matching.Regex = (^|\/)\.[^\/\.]

scala> def test(s: String) = HiddenPattern.findFirstIn(s).isDefined
def test(s: String): Boolean

scala> test("/var/folders/13/gsqmg365255bnrlnmn7m6pfr0000gn/T/GlfN5L/.should-be-hidden/not-hidden")
val res2: Boolean = true

scala> test("/var/folders/13/gsqmg365255bnrlnmn7m6pfr0000gn/T/GlfN5L/should-be-hidden/not-hidden")
val res3: Boolean = false

Made a test for it which checked for true "if the path is for a non-hidden file in a hidden directory", and it passed. But it failed in the JVM: as far as I understand, in JVM, only the file itself is checked to be hidden, so if it's not hidden itself, it doesn't matter if it is located somewhere in a hidden directory. So that test is now checking for false.

Having that, I removed the regex completely, leaving a simple implementation instead:

    override def isHidden(path: Path): F[Boolean] = F.pure {
      val fileName = path.fileName.toString
      fileName.length >= 2 && fileName(0) == '.' && fileName(1) != '.'
    }

I'm uncomfortable with doing head-like things, but doing something like fileName.length >= 2 && fileName.headOption.contains('.') && !fileName.drop(1).headOption.contains('.') feels like an overkill (and a bunch of unnecessary allocations).

@yurique
Copy link
Contributor Author

yurique commented Nov 30, 2021

As for testing the .isHidden in Windows... I don't even know, I'm hurting from even trying to think about that :) (and we don't have Windows CI)

(also this https://stackoverflow.com/questions/53791740/why-does-files-ishiddenpath-return-false-for-directories-on-windows)

Copy link
Member

@armanbilge armanbilge left a comment

Choose a reason for hiding this comment

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

I'm really thrilled about these fixes, and overall the improved test coverage for the Files API. 🤞 this will fix some other issues downstream as well :) thanks!

}
}

group("isHidden") {
Copy link
Member

Choose a reason for hiding this comment

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

Even though there's no windows CI, this will be annoying for anyone developing on Windows. Not sure how big a deal that is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah 🤔
If someone's working on fs2 in Windows and runs the tests.. that will be annoying (I kind of blanked out such a possibility :/ )
I wonder if we can do some platform checks in the test code and, at least, always pass isHidden true-case if it's windows?

Copy link
Member

Choose a reason for hiding this comment

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

Eh, it's merged. Let's see if anyone complains :P

The problem is we need cross-platform (JVM/JS) checks for checking the platform (Unix/Windows). Possible, but annoying!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Eh, it's merged. Let's see if anyone complains :P

=)

The problem is we need cross-platform (JVM/JS) checks for checking the platform (Unix/Windows). Possible, but annoying!

Should be non-crazy enough to implement, right?

Is there a good place for it? (like Files[F] is for the file stuff)

Copy link
Member

Choose a reason for hiding this comment

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

Oh, as a public API? Probably not in fs2. I was thinking as an internal/test utility.

@mpilquist mpilquist merged commit 84d87a5 into typelevel:main Nov 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants