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

"one" </> "/two" == "/two" needs explanation #49

Open
wereHamster opened this issue Jun 24, 2015 · 21 comments
Open

"one" </> "/two" == "/two" needs explanation #49

wereHamster opened this issue Jun 24, 2015 · 21 comments

Comments

@wereHamster
Copy link

Nothing in the documentation indicates that behavior: http://hackage.haskell.org/package/filepath-1.4.0.0/docs/System-FilePath-Posix.html#v:-60--47--62-

@ndmitchell
Copy link
Contributor

So the docs for </> point at combine, but I agree a few more examples at </> are wise. Unfortunately the docs for combine have been misrendered, which makes it harder to see the examples as well. That's very unfortunately, I should fix both issues.

@wereHamster
Copy link
Author

It would be nice if the package exposed a function which combines the two paths as "one/two". There is such a function internally (combineAlways), but it is not exported. I use joinDrive which is a synonym for combineAlways. But that workaround makes my code more complicated to understand.

@ndmitchell
Copy link
Contributor

What are you using that function for? Any examples? I'm not totally against it, but I'd like to understand in what circumstances it should be recommended.

@wereHamster
Copy link
Author

I have a function which may return an absolute URL path (eg. /assets/app-1234.js). I need to take that path and write the file contents to the filesystem under a base path (eg. dist/). This base path may or may not be absolute and may or may not have a trailing slash.

So, in Haskell language:

dump :: FilePath -> FilePath -> ByteString -> IO ()
dump basePath assetPath body = writeFile (basePath </> assetPath) body

main = do
    dump "./dist/" "/assets/app-1234.js" "..."

I would rather have two functions which are clearly documented than one which tries to guess what my intentions are.

@rwbarton
Copy link

Without having examined all the examples for combine closely, I think it has a single clear purpose, though the documentation doesn't really state it. That purpose is:

If path1 names a directory B relative to a directory A, and path2 names an object (file or directory) C relative to directory B, then combine path1 path2 names the object C relative to directory A. Or in other words, to use a weird shell/Haskell hybrid, (cd path1 && cat path2) should cat the same file as cat $(combine path1 path2).

@wereHamster's operation I think is best viewed as splitting the absolute path /assets/app-1234.js into a relative path assets/app-1234.js on top of the absolute path /, then combining that relative path with the bast path ./dist/. In general this operation is useful even when splitting off an absolute path other than /; for example you might have your web server configured to serve /assets/ from a directory that is not named assets, and need to split off /assets/. In general you might want an equivalent to Data.List's stripPrefix for this purpose, though I guess makeRelative and isRelative handle this pretty well. In the case of / you can obviously write it in a number of simple ways depending on how you want to handle the input not actually being an absolute pathname.

@thomie
Copy link
Contributor

thomie commented Aug 18, 2015

Maybe we should rewrite combineAlways a bit, and enforce the following rules:

combineAlways x y == x </> (dropDrive y)
Valid x y => isValid (combineAlways x y)

It would prevent current invalid results such as:

combineAlways "C:/" "D:/foo" == "C:/D:/foo"

@wereHamster: please note that joinDrive is only a synonym for combineAlways since filepath-1.4.0.

From the 1.4 changelog:

  • Semantic change: joinDrive /foo bar now returns /foo/bar, instead of /foobar

Edit: a consumer of this new version of combineAlways would be Cabal, in the file Cabal/Distribution/Simple/InstallDirs.hs, when installing a library with the --prefix configure option:

CopyTo destdir -> fmap ((destdir </>) . dropDrive)

@ndmitchell
Copy link
Contributor

I've clarified a lot, added the intuition from @rwbarton (in Haskell form), put all the docs under </> and included the example that spawned this bug report. I would recommend that if you want to splice URL's in this way you do:

 basePath </> dropWhile (== '/') assetPath

I don't think exposing combineAlways is a good idea - it's very much an internal detail, and one that I'm not convinced will remain consistent forever. Alternatively, define your own:

x +/+ y = x ++ "/" ++ y

This is a lexical operation, rather than a filepath one.

@ndmitchell
Copy link
Contributor

My inclination is to close this ticket now the docs are fixed - anyone still think it would be good to have "something else", and if so, what that something else should do (not in terms of code, more the idea behind it).

@rwbarton
Copy link

I think the new comment might be a bit easier to read if you swap the two Haskell bits; then it reads more like a definition of </>, which is what you are documenting. And yes I see I wrote it the "wrong way" originally :)

Aside from that, I agree with closing this ticket.

@ndmitchell
Copy link
Contributor

You mean make it:

The intention is that readFile (dir </> file) will access the same file as setCurrentDirectory dir; readFile file.

? If that seems clearer to you, I'll go with that.

@rwbarton
Copy link

Yes, exactly.

@ndmitchell
Copy link
Contributor

Done, thanks for the suggestion.

@hasufell
Copy link
Member

The same problem applies to joinPath where joinPath ["/foo", "/"] == "/" which isn't documented either. I think that behavior is wrong altogether, but it should at least be consistently documented so haskell code doesn't accidentally try to remove your whole root file system, because somewhere down the call stack System.FilePath caused semi-defined behavior.

@mat8913
Copy link

mat8913 commented Dec 11, 2017

Doesn't the new comment conflict with "C:\\home" </> "\\bob" == "\\bob"?

@hasufell
Copy link
Member

hasufell commented Dec 3, 2021

So this behavior seems already sufficiently documented.

I'm providing a PR for discussion, which exposes an alternative (<\>) that is essentially the already existing combineAlways: #97

@hasufell
Copy link
Member

hasufell commented Dec 4, 2021

I also just discovered:

Prelude System.FilePath.Windows> "lol" </> "/bar"
"/bar"

This is incorrect on windows, since /bar on windows is a relative path:

Prelude System.FilePath.Windows> isRelative   "/bar"
True

But from what I understand it's relative to the current device?

@ndmitchell
Copy link
Contributor

On Windows you have drive-relative paths and path-relative paths. /bar is drive relative, so if you are on C: it's C:/barand if you are onD:it'sD:/bar. That means its not fully-relative, but definitely not absolute, and in that case is right to return not . isAbsolute. Even though it's relative, if you were in the loldirectory and didcd /bar, you'd end up in /bar, so that's the right answer. If you were in C:/loland didcd /baryou'd get toC:/bar` , which is not what FilePath returns. But in the corner case of somewhat relative paths, I don't think I'd term any answer as unambiguously wrong, but more a case of interpretation.

@ndmitchell
Copy link
Contributor

FWIW, I still think exposing combineAlways is a mistake. I don't think it does what anyone wants and seems like a foot gun. But I'm no longer maintainer of this library, so if you want to, it's your choice.

@hasufell
Copy link
Member

On Windows you have drive-relative paths and path-relative paths. /bar is drive relative, so if you are on C: it's C:/barand if you are onD:it'sD:/bar. That means its not fully-relative, but definitely not absolute, and in that case is right to return not . isAbsolute. Even though it's relative, if you were in the loldirectory and didcd /bar, you'd end up in /bar, so that's the right answer. If you were in C:/loland didcd /baryou'd get toC:/bar` , which is not what FilePath returns. But in the corner case of somewhat relative paths, I don't think I'd term any answer as unambiguously wrong, but more a case of interpretation.

I think one solution could be to return a sum type of cardinality 3, then mark isAbsolute/isRelative as deprecated (for a couple years or indefinitely).

My opinion is that we should ultimately define platform specific functions that are unambiguous.

The platform agnostic module could then:

  1. Export only the strictly common functions and
  2. Define semantically less strict functions

@ndmitchell
Copy link
Contributor

My opinion is that we should ultimately define platform specific functions that are unambiguous.

I disagree, since I think that's a good way to have none of the ecosystem work on Windows. In my view, working transparently across platforms is way more important than high fidelity. But its your choice now :)

@hasufell
Copy link
Member

My opinion is that we should ultimately define platform specific functions that are unambiguous.

I disagree, since I think that's a good way to have none of the ecosystem work on Windows. In my view, working transparently across platforms is way more important than high fidelity. But its your choice now :)

We already expose platform specific functions via the qualified modules. If someone imports them without understanding the consequences, they already break cross platform support.

The idea is rather that those platform specific modules can have more specific functions (not existing in the "cross" module) that people can use when implementing their own platform specific logic. I don't believe we will ever be able to cover all of the use cases, as the relative path issues show.

Now, users who want more details about their windows paths, could easily get it without re-implementing stuff, which is rather error prone.

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

No branches or pull requests

6 participants