update `splitDirectories` function by d12frosted · Pull Request #4 · fpco/haskell-filesystem

d12frosted · 2015-01-26T14:44:07Z

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 · 2015-01-26T19:49:07Z

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 · 2015-01-26T20:08:30Z

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.

so it respects root and directories

snoyberg · 2015-01-27T18:22:09Z

@chrisdone Can you look into this?

d12frosted · 2015-02-02T15:45:13Z

Ping. Any updates on this?

chrisdone · 2015-02-02T17:09:40Z

Checking it out.

chrisdone · 2015-02-02T17:59:12Z

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 · 2015-02-02T18:01:58Z

Ah, from the test suite.

chrisdone · 2015-02-02T18:19:29Z

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

Passing to @snoyberg.

d12frosted · 2015-02-03T09:24:43Z

@chrisdone thanks for checking my commit

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

True indeed.

update `splitDirectories` function

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

update splitDirectories function

718ba6f

so it respects root and directories

snoyberg assigned chrisdone Jan 27, 2015

chrisdone assigned snoyberg and unassigned chrisdone Feb 2, 2015

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

Merge pull request #4 from d12frosted/master

620a705

update `splitDirectories` function

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

d12frosted mentioned this pull request Feb 9, 2015

concat . splitDirectories is not the id function? #3

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

update `splitDirectories` function#4

update `splitDirectories` function#4
snoyberg merged 1 commit intofpco:masterfrom
d12frosted:master

d12frosted commented Jan 26, 2015

Uh oh!

d12frosted commented Jan 26, 2015

Uh oh!

d12frosted commented Jan 26, 2015

Uh oh!

snoyberg commented Jan 27, 2015

Uh oh!

d12frosted commented Feb 2, 2015

Uh oh!

chrisdone commented Feb 2, 2015

Uh oh!

chrisdone commented Feb 2, 2015

Uh oh!

chrisdone commented Feb 2, 2015

Uh oh!

chrisdone commented Feb 2, 2015

Uh oh!

d12frosted commented Feb 3, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

d12frosted commented Jan 26, 2015

Uh oh!

d12frosted commented Jan 26, 2015

Uh oh!

d12frosted commented Jan 26, 2015

Uh oh!

snoyberg commented Jan 27, 2015

Uh oh!

d12frosted commented Feb 2, 2015

Uh oh!

chrisdone commented Feb 2, 2015

Uh oh!

chrisdone commented Feb 2, 2015

Uh oh!

chrisdone commented Feb 2, 2015

Uh oh!

chrisdone commented Feb 2, 2015

Uh oh!

d12frosted commented Feb 3, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants