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

update `splitDirectories` function #4

Merged
merged 1 commit into from Feb 8, 2015

Conversation

Projects
None yet
3 participants
@d12frosted
Copy link
Contributor

d12frosted commented Jan 26, 2015

I propose this solution for issue #3.

In short, currently concat . splitDirectories /= id.

Here is example of using concat . splitDirectories:

> Filesystem.Path.concat $ splitDirectories "/path/to/file"
FilePath "//path/to/file"

In my commit I updated splitDirectories function, so now it treats root part as pathRoot, but not as pathBasename.

I have run all testes, and here is content of log file:

Test suite filesystem_path_tests: RUNNING...
PASS: 47 tests run, 47 tests passed
Test suite filesystem_path_tests: PASS
Test suite logged to: dist/test/system-filepath-0.4.13.1-filesystem_path_tests.log

I found that there is test_SplitDirectories function in FilesystemPathTests.hs file:

test_SplitDirectories :: Test
test_SplitDirectories = assertions "splitDirectories" $ do
    let splitDirectories x = map toChar8 (P.splitDirectories (fromChar8 x))

    $expect $ equal (splitDirectories "") []
    $expect $ equal (splitDirectories "/") ["/"]
    $expect $ equal (splitDirectories "/a") ["/", "a"]
    $expect $ equal (splitDirectories "/ab/cd") ["/", "ab", "cd"]
    $expect $ equal (splitDirectories "/ab/cd/") ["/", "ab", "cd"]
    $expect $ equal (splitDirectories "ab/cd") ["ab", "cd"]
    $expect $ equal (splitDirectories "ab/cd/") ["ab", "cd"]
    $expect $ equal (splitDirectories "ab/cd.txt") ["ab", "cd.txt"]
    $expect $ equal (splitDirectories "ab/cd/.txt") ["ab", "cd", ".txt"]
    $expect $ equal (splitDirectories "ab/./cd") ["ab", ".", "cd"]

So probably it couldn't find the problem because of toChar8 . f . fromChar8. Basically, that's what I get if I don't convert toChar8:

> splitDirectories (fromChar8 "/")  == [fromChar8 "/"]
False
> Prelude.map toChar8 (splitDirectories (fromChar8 "/"))  == Prelude.map toChar8 [fromChar8 "/"]
True

Also I see that there are other places where toChar8 . f . fromChar8 conversion is used, so I am going to check those functions as well.

Sorry for long message.
Cheers and thanks for your awesome library!

@d12frosted

This comment has been minimized.

Copy link
Contributor Author

d12frosted commented Jan 26, 2015

I checked the test function. And here are my thought about how to update it.

My first attempt was to compare lists of FilePaths:

test_SplitDirectories :: Test
test_SplitDirectories = assertions "splitDirectories" $ do
        let splitDirectories x = P.splitDirectories (fromChar8 x)
            fromChar8' = map fromChar8

        $expect $ equal (splitDirectories "") (fromChar8' [""])
        $expect $ equal (splitDirectories "/") (fromChar8' ["/"])
        $expect $ equal (splitDirectories "/a") (fromChar8' ["/", "a"])
        $expect $ equal (splitDirectories "/ab/cd") (fromChar8' ["/", "ab", "cd"])
        $expect $ equal (splitDirectories "/ab/cd/") (fromChar8' ["/", "ab", "cd"])
        $expect $ equal (splitDirectories "ab/cd") (fromChar8' ["ab", "cd"])
        $expect $ equal (splitDirectories "ab/cd/") (fromChar8' ["ab", "cd"])
        $expect $ equal (splitDirectories "ab/cd.txt") (fromChar8' ["ab", "cd.txt"])
        $expect $ equal (splitDirectories "ab/cd/.txt") (fromChar8' ["ab", "cd", ".txt"])
        $expect $ equal (splitDirectories "ab/./cd") (fromChar8' ["ab", ".", "cd"])

But cabal test showed me

equal: [FilePath "ab",FilePath ".",FilePath "cd"] is not equal to [FilePath "ab",FilePath "./",FilePath "cd"]

So yeah. that's was expectable. The second attempt was to use concat . splitDirectories:

test_SplitDirectories :: Test
test_SplitDirectories = assertions "splitDirectories" $ do
    let splitDirectories x = P.concat $ P.splitDirectories (fromChar8 x)

    $expect $ equal (splitDirectories "") (fromChar8 "")
    $expect $ equal (splitDirectories "/") (fromChar8 "/")
    $expect $ equal (splitDirectories "/a") (fromChar8 "/a")
    $expect $ equal (splitDirectories "/ab/cd") (fromChar8 "/ab/cd")
    $expect $ equal (splitDirectories "/ab/cd/") (fromChar8 "/ab/cd")
    $expect $ equal (splitDirectories "ab/cd") (fromChar8 "ab/cd")
    $expect $ equal (splitDirectories "ab/cd/") (fromChar8 "ab/cd")
    $expect $ equal (splitDirectories "ab/cd.txt") (fromChar8 "ab/cd.txt")
    $expect $ equal (splitDirectories "ab/cd/.txt") (fromChar8 "ab/cd/.txt")
    $expect $ equal (splitDirectories "ab/./cd") (fromChar8 "ab/./cd")

(yeah, probably I should use another name instead of splitDirectories)

All tests are passed and I am happy.

But wait. While I do like using concat . splitDirectories == id property I am concerned about previous equality failure.

My intuition says me that splitDirectories "ab/./cd" should be equal to fromChar8' ["ab", ".", "cd"].

Current definition of splitDirectories function treats all dirs as basenames. And that's probably not what we want.

Checkout my last commit to see what I've got.

@d12frosted d12frosted changed the title splitDirectories now treat root as root update `splitDirectories` function Jan 26, 2015

@d12frosted

This comment has been minimized.

Copy link
Contributor Author

d12frosted commented Jan 26, 2015

Hm, just checked cabal test for few times and found:

[ FAIL  ] tests.validity.windows
  note: seed=4823750214036955069

    *** Failed! Falsifiable (after 30 tests): 
FilePath "c\FS2H/\ENQ\DELQ\SI\135/\223RO&\244\131\151:&/t%f/Lt\167\194\176\199\GSL `/c/{\SO/`M/\a/\172\241^:/>~\172`d8T\252/{/\166c\DLE\142\rN>!/<\176\fy_\134K\a\DLE\US/0Bv%u \EMD/L\ESCc\DC1^/\237Ra\233\FS/b\206y</!/J/\249nT\195/\DEL/>P*\SI\171V9/>j'Fy;\FS/\165\&4\DLE\157q}pi5/B\209h<\201g(2'/Q\163\t<X/*\242\143Y\SOH\230P/|/\148\GS~Y/\136r\166\142"

But I saw it only once.

Boris
update splitDirectories function
so it respects root and directories
@snoyberg

This comment has been minimized.

Copy link
Member

snoyberg commented Jan 27, 2015

@chrisdone Can you look into this?

@d12frosted

This comment has been minimized.

Copy link
Contributor Author

d12frosted commented Feb 2, 2015

Ping. Any updates on this?

@chrisdone

This comment has been minimized.

Copy link
Member

chrisdone commented Feb 2, 2015

Checking it out.

@chrisdone

This comment has been minimized.

Copy link
Member

chrisdone commented Feb 2, 2015

So there's quite a mess of invariants flying around in this library. But the ./ stuff is already a problem, seemingly regardless of this patch.

Where are your fromChar8/toChar8 coming from?

@chrisdone

This comment has been minimized.

Copy link
Member

chrisdone commented Feb 2, 2015

Ah, from the test suite.

@chrisdone

This comment has been minimized.

Copy link
Member

chrisdone commented Feb 2, 2015

Okay, this gives at least the property of identity on concat . splitDirectories. Okay to merge from my side.

Passing to @snoyberg.

@chrisdone chrisdone assigned snoyberg and unassigned chrisdone Feb 2, 2015

@d12frosted

This comment has been minimized.

Copy link
Contributor Author

d12frosted commented Feb 3, 2015

@chrisdone thanks for checking my commit

But the ./ stuff is already a problem, seemingly regardless of this patch.

True indeed.

snoyberg added a commit that referenced this pull request Feb 8, 2015

Merge pull request #4 from d12frosted/master
update `splitDirectories` function

@snoyberg snoyberg merged commit 620a705 into fpco:master Feb 8, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.