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

doesDirectoryExist "" changed behaviour #84

Closed
dbdbdb opened this issue Jul 9, 2018 · 4 comments
Closed

doesDirectoryExist "" changed behaviour #84

dbdbdb opened this issue Jul 9, 2018 · 4 comments
Assignees
Labels
type: a-bug The described behavior is not working as intended.

Comments

@dbdbdb
Copy link

dbdbdb commented Jul 9, 2018

doesDirectoryExist for the empty string has changed behaviour and now say yes. I have some old code that stopped working when updating this lib.

Couldn't see it documented so I guess it is a bug?

@hvr
Copy link
Member

hvr commented Jul 9, 2018

A quick bisecting over releases revelead this change occured between directory-1.3.1.0 (returns False) and directory-1.3.1.1 (returns True)

@Rufflewind
Copy link
Member

@dbdbdb: Thanks for catching this.

@hvr: Thanks for looking into this.

This was introduced in 5eb35cf . I prefer it this way because it is more consistent with how most things are done in directory: "" is interpreted as "." as with normalise, although I probably underestimated its impact when I made that change.

At this point I don't think I will be reverting this change (though you can try to convince me otherwise!). Rather, I would argue if a directory function that rejects "", then that should be considered a bug.

@dbdbdb
Copy link
Author

dbdbdb commented Jul 10, 2018

I've never seen that semantic in any other language so it would come as a surprise for most I guess.

For example these languages return False for "" and True for ".":

Bash: [ -d "" ]
Python: os.path.isdir("")
PHP: is_dir("")
Perl: -d ""
Ruby: File.directory?("")
C/C++: stat("", &s) followed by S_ISDIR(s.st_mode) (for empty string stat() return ENOENT)
Java: file = new File("") followed by file.isDirectory()

So it should at least be clearly documented.

@Rufflewind
Copy link
Member

Okay, upon further thought, I decided to revert the change, mainly because operating systems do not treat "" and "." as the same. For example, setCurrentDirectory "" fails and I think it's better to leave it that way.

It's unfortunate that some of the existing API (i.e. canonicalizePath, makeAbsolute, timestamps, and permissions) do accept "" but I think being more lenient than the OS is not a major problem for these functions. In contrast, users will likely use pathIsDirectory and doesDirectoryExist to validate paths, so being too lenient is more likely to be an issue.

I'm making some significant refactoring because of this, but the only user-facing change should be that pathIsDirectory and doesDirectoryExist will no longer accept empty paths, as they used to be. In the code of directory, it will be more obvious which functions do accept empty paths (because they will call emptyToCurdir). Due to various flaws in normalise, nearly all uses have been replaced with System.Directory.Internal.Common.simplify instead.

@Rufflewind Rufflewind self-assigned this Dec 17, 2018
@Rufflewind Rufflewind added the type: a-bug The described behavior is not working as intended. label Dec 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: a-bug The described behavior is not working as intended.
Projects
None yet
Development

No branches or pull requests

3 participants