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

Support for relative paths or documentation of differences to Path behaviour #89

Closed
jotomo opened this issue Oct 29, 2016 · 8 comments
Closed
Labels

Comments

@jotomo
Copy link
Contributor

jotomo commented Oct 29, 2016

I'm in the process of moving from Java NIO to this library. File's Javadoc state it's a wrapper around java.nio.Path, so I was expecting the same semantics. However, File does not support relative paths, but converts everything to absolute paths during instantiation. This leaves me puzzled ...

If this behaviour is by design, I think it should be documented.

P.S. Why does File.nameWithoutExtension does I/O, checking a file is either a regular file or not existing? This was also surprising when transferring assumptions over from the behaviour of Path and caused a subtle bug in my test: the file in question was in a directory to which the user had no access, so File.hasExtension returned false, so that File.nameWithoutExtension simply returned the name with the extension.

@pathikrit pathikrit added the bug label Nov 10, 2016
@pathikrit
Copy link
Owner

pathikrit commented Nov 10, 2016

You are probably right that File.nameWithoutExtension should not do I/O but I disagree that this library should strive to mimic nio.Path in general.

@jotomo
Copy link
Contributor Author

jotomo commented Nov 10, 2016

To be clear: I'm not saying better-files should mimic nio.Path, but in the absence of docs that state how better-files behaves, or differs from nio.Path, it seems a natural assumption. So maybe add a note to the Instantiation section to state that all paths are normalized to absolute paths during instantiation?

@pathikrit
Copy link
Owner

@jotomo : Yes, plan to strongly-type the paths anyway (see #88 (comment))

But, I have a separate question for the File.hasExtension (which is the culprit that does I/O when nameWithoutExtension is called). Should hasExtension return false if we know the file is a directory? If yes, to check if a file is a directory, doing I/O is nessecary. If no, then for files that do exist on the file-system and are directories we will return a non-sensical "true".

I think we need to distinguish between a "vitrual file" vs. "a real file that exists on the file system".

@jotomo
Copy link
Contributor Author

jotomo commented Nov 10, 2016

The more I think about this, the trickier the separation between just manipulating a File instance vs. doing I/O gets. isDirectory needs to do I/O, so hasExtension should be allowed to do I/O as well when required (I'm aware I'm arguing against my initial statement).
The question, I think, is how smart hasExtension shoud be. When not doing I/O it would just check for a dot in the filename, requiring the user to check the File doesn't point to a directory, using isDirectory which in turn does I/O. When doing I/O, and being clever, the method should probably handle any error in accessing the file the same way as when the file doesn't exist, otherwise it would become a method that checks if it's a directory, if it is readable ... and gets complex quickly.
Generally, I don't see why directories can't have extensions, e.g. I sometimes use .bak when I create backups of entire directories, but that's probably more confusing than helping in the general case (I have my local copy patched to have hasExtension (and changeExtensionTo) operate an files and directories regardless).
So, using the principle of least surprise as guidance and saying only files have extension the behaviour would be that hasExtension checks if the physical file is a directory to return false in that case (rather than a non-sensical true like you say), regardless of any dots in the name. Any I/O error occurring during that check should be swallowed and the return value should be based on the File instance alone. Does that make sense?
There might be a use-case where one is constructing File instances for non-existing files and uses hasExtension, which would return false unexpectedly, if the File instance has a dot in the name, and on disk there just happens to be a directory by that very name already .... but I'm likely overthinking things at this point :-)

@pathikrit
Copy link
Owner

In that case, the current behavior is correct:

def hasExtension: Boolean =
    (isRegularFile || notExists) && (name contains ".")

The above will return true if name has a . AND either it exists on the file system and is a vanilla file (i.e. not a directory or a process file etc) OR it does not exist on the file system (the "virtual" file case).

@jotomo
Copy link
Contributor Author

jotomo commented Nov 11, 2016

Agreed. I think my original irritation was misunderstanding the behaviour of nio.Files.notExists (and neighbouring methods), or rather not thinking about the underlying Java code, which swallows exceptions and returns false when "the state of the file can't be determined". The effect, when working with a file one isn't allowed to access has these results then:

File("/root/test.txt").hasExtension => false
File("/root/test.txt").nameWithoutExtension => "test.txt"
Files.isRegularFile(Paths.get("/root/test.txt")) => false
Files.isDirectory(Paths.get("/root/test.txt")) => false
Files.notExists(Paths.get("/root/test.txt")) => false
Files.exists(Paths.get("/root/test.txt")) => false
Files.isReadable(Paths.get("/root/test.txt")) => false

I don't have an answer as to what the behaviour should be. I think though that I need to make my code more resilient and maybe start with isReadable when there's a chance a permission problem might exist. Hm, hasExtension could start with an isReadable maybe, not sure though if this is the right place to address those issues.

@pathikrit
Copy link
Owner

Well atleast these 2 are consistent:

File("/root/test.txt").hasExtension => false
File("/root/test.txt").nameWithoutExtension => "test.txt"

I think the resolution for this is to document the fact that for unreadable files, we assume it is not a "regular file" and thus has no extension.

@pathikrit pathikrit added question and removed bug labels Nov 12, 2016
@jotomo
Copy link
Contributor Author

jotomo commented Nov 12, 2016

👍 Agreed. Thanks for condensing my ponderings into something actionable :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants