Move ‘dirname’ and ‘parent’ into ‘MonadThrow’

[mrkkrp]

Close #18.

[mrkkrp]

@NorfairKing Can you adjust the validity tests?

[NorfairKing]

Yes! Dirname doesn't have validity tests yet though, so I'll add those.

[NorfairKing]

@mrkkrp Can you update the docs as well?

[NorfairKing]

And could you also check the property for 'parent' because it doesn't seem to hold:

     it "(parent (x </> y) == x)" $ do                                                                      
        forAllShrink genValid shrinkValidAbsDir $ \x ->                                                     
            forAllShrink genValid shrinkValidRelFile $ \y ->                                                
                parent (x </> y) == Just x                 

  1) Operations: parent (parent (x </> y) == x)
       Falsifiable (after 5 tests and 5 shrinks): 
       "/"
       "a/a"

[mrkkrp]

parent is still idempotent, so it does not need to be in MonadThrow. We should probably either make it throw when it's applied to "/" paths or leave it as is and move it outside of MonadThrow... I think in #33 we concluded that it should throw on "/". I'll implement this now.

[mrkkrp]

Does everyone agree with the decision about parent? I think we should add Couldn'tTakeParentOfRoot data constructor to PathParseException. What do you think?

[NorfairKing]

Sounds great! Feel free to add the tests as well then .

[sjakobi]

I suspect that this line is redundant. Would you mind removing it and checking if everything still seems to work as expected?

[sjakobi]

Does everyone agree with the decision about parent?

I do.

I think we should add Couldn'tTakeParentOfRoot data constructor to PathParseException. What do you think?

That constructor doesn't really sound like a "parse exception" but PathParseException is already a mixed bag so I don't mind adding it there.

---

Can you add tests for when parent and dirname fail please?!

[sjakobi]

Oh, and how about adding @since annotations on dirname and parent? IMO that makes sense for a breaking type change.

BTW in the changelog you can substitute UNRELEASED with 0.6.0 now that we have a breaking change.

[mrkkrp]

AppVeyor fails with a weird message (doesn't know what curl is!).

[sjakobi]

AppVeyor fails with a weird message (doesn't know what curl is!).

Seems like a new AppVeyor issue – can you apply the workaround here?

[mrkkrp]

We should instead use shouldThrow here and check that it throws the correct thing, not discard the info checking against Nothing. I'll push a fix.

[NorfairKing]

The constructors of the exception are not supported, are they?
If they are, then I missed that :)

[mrkkrp]

It looks like we don't export constructors of PathParseException. How about exposing them?

[NorfairKing]

How about exposing them?

Yes please.

[mrkkrp]

@sjakobi Good to go now, what do you think?

[NorfairKing]

@mrkkrp also do that on windows then.
I also think that using 'Either' would be more appropriate in the tests.
.. and you might want to add this test for windows as well: ba591bb

[mrkkrp]

also do that on windows then.

Do what? The tests on Posix and Windows are identical now I think, except for ba591bb.

Oh, yes they are. My bad.

I also think that using 'Either' would be more appropriate in the tests.

Again, not sure what you mean by this.

parent $(mkAbsDir "/") `shouldBe` Right (Couldn'tTakeParent "/")

[NorfairKing]

Otherwise, LGTM

[mrkkrp]

Please don't add your answers in my post by editing it, it's confusing. And this

parent $(mkAbsDir "/") `shouldBe` Right (Couldn'tTakeParent "/")

is not going to work:

 (~) * e SomeException => MonadThrow (Either e)

So it will be Left with SomeException inside, which we will need to convert to PathParseException in order to check for equality. Nothing nice in that approach.

[NorfairKing]

Please don't add your answers in my post by editing it, it's confusing.

Oh dear! I didn't know I was doing that. I'm so sorry!

So it will be Left with SomeException inside, which we will need to convert to PathParseException in order to check for equality. Nothing nice in that approach.

Oh, right. Carry on! LGTM from me :)

[sjakobi]

I'm sorry that this comes so late, but I've got some doubts about our (type) design here:

• I think that for Path Abs File we should have a pure parent function.
  What do you think about replacing parent with these functions?

  dirParent :: MonadThrow m => Path b Dir -> m (Path b Dir)
  fileParent :: Path Abs File -> Path Abs Dir
  

  Once we can represent the current directory (.) as a Path Rel Dir we could generalize fileParent to Path b File -> Path b Dir.

  With this design we could even keep the old parent function although we should document its inconsistency with isParentOf.

• I think that throwing InvalidRelDir from dirname "breaks the abstraction". It also seems to me that we'll always throw InvalidRelDir "" there which will make it difficult for users to find the cause of the exception.
  How about having a Couldn'tTakeDirname HasNoDirname constructor instead?

cc @harendra-kumar

[NorfairKing]

I think that for Path Abs File we should have a pure parent function.

+1

[harendra-kumar]

@mrkkrp @sjakobi PR #35 which was reviewed by @chrisdone long ago but was never merged (I guess he got busy), fixes the parent related part in this PR. There may be a few things from the changes in that PR that may be worth taking or maybe you can merge that one first. That PR changed the exception naming as well, you may want to take a look at the review comments in that one.

I agree InvalidRelDir thrown from dirname is confusing, we should perhaps have a specific exception for dirname. BTW, I had another suggestion for fixing dirname in #35, a little unusual but works. With #34 we have an empty path as identity of concatenation for Path Rel Dir, the same can be used as dirname for /. In other words, dirname / could be . because /. is same as / and a/. is same as a/.

[sjakobi]

dirname / could be .

That just doesn't fit with my intuition of what a "directory name" is. I really think that dirname / should fail.

[harendra-kumar]

I will say don't add them (i.e. fileParent and dirParent) now, adding later is always easier, if we really want to, but removing later is painful.

[NorfairKing]

If we don't split parent now, and we move it into MonadThrow, then splitting it into fileParent and dirParent will require ANOTHER major release.
I really need fileParent in the case of file to be pure.
We could keep parent as is (including it's functionality) and also add fileParent and dirParent. I would be fine with that, but we have already established that parent is broken, so it doesn't sound great.

However, if we do split then we should perhaps keep parent as well for the case when you do not care whether the path is file or dir.

You never "don't care" whether a path is a file or a dir, you can only "not know" and write a function that works for both. If you use parent for that, it's already been established that your function will be broken.
There is simply no way to write the function you are describing correctly, if parent is not in MonadThrow.

[...] because the added cognitive overhead by splitting cancels the advantage of not having a MonadThrow in case of a file

I disagree entirely. The opposite is true, in my opinion. The cognitive overhead of not splitting parent is not worth it.
I want to be able to write pure code that uses fileParent. If this library is going to force me to write the following code, then something is very wrong:

case parent myFile of
    Nothing -> error "cannot happen anyway"
    Just parentOfMyFile -> pure $ myFunc parentOfMyFile

Types should not lie.

[mrkkrp]

If we move parent into MonadThrow then it's not broken anymore and there is no problem with it being in the library. parent already works as dirParent, so dirParent (if we choose to keep parent) is redundant. We can add fileParent like this:

fileParent :: Path Abs File -> Path Abs Dir
fileParent = fromJust . parent

But again if you need it so badly you can do it yourself in your code. And yes fromJust is OK here, it's silly to be some sort of purist and reject solutions that work perferctly in all cases only because somewhere is branch of execution that is never reached there is an error.

[NorfairKing]

We can add fileParent like this [...]

I'm okay with that.

The difference is who maintains it. As long as fileParent is in path and not in my own code, I'm happy with it. fromJust or not, that's an implementation detail, as long as it's tested well.

[NorfairKing]

Are we ready to merge this then?

[mrkkrp]

I think we're close. @sjakobi mentioned that he suspects that dirname will always throw the same exception: InvalidRelDir "". We should check this and then it will be ready. I can check that tomorrow.

[harendra-kumar]

With this fix we are moving towards an unnecessarily complex API. Let me explain.

Just like the POSIX standard path API, our path API could be as simple as just two pure functions i.e. parent (called dirname in POSIX) and basename (we have dirname and filename which can be easily combined together in basename). So in entirety we have:

(</>) :: Path b Dir -> Path Rel t -> Path b t
parent   :: Path b t -> Path b Dir
basename :: Path b t -> Path Rel t

This is succinct and simple API, look how the signatures of all three fit in. And, we have the following simple laws too:

parent x </> basename x == x
basename (x </> y) == basename y
parent (x </> basename y) == x

With MonadThrow instances and splitting the API we are moving away from this simpler picture and towards a more ad-hoc API. To be able to keep this simpler picture I propose the following semantics for the special root case, assuming we also have the change to represent the empty path or ".":

parent "/" = "/"
basename "/" = "."

With these semantics all the laws are elegantly satisfied and everything is still pure. We already have the first of these, the only change we need is the second one. We discussed this earlier above and dismissed as non-intuitive, but it makes sense and fulfills the laws. If you think about it, the basename of "/" is in fact an empty name and that is what we represent with "." (it is Path []). You just have to reconcile with this one thing and we have a simple, law abiding and beautiful API which also happens to be inline with the POSIX standard. These semantics take care of #18, of course.

I request all the maintainers to take another look at this and strive to keep the API as simple as possible, since this is a fundamental library for path manipulations. I use and care about this library, so I tried to make a bit more effort to convince you guys.

[harendra-kumar]

BTW, I have a change that combines dirname and filename into a polymorphic basename API using a type class.

[NorfairKing]

I'm sort-of okay with basename "/" = ".", not so much with parent "/" = "/".
I'm also unsure about the type class.
Other than that, I could be convinced.
Would like to hear other opinions.

[joehillen]

What is wrong with parent "/" = "/"?

Seems perfectly reasonable to me. If you type cd .. in the shell at /, it sends you to /.

[NorfairKing]

What is wrong with parent "/" = "/"?

I can get my head around it, as long as isParentOf is consistent. see #33.
As for basename "/" = ".", that would solve #18 without MonadThrow 👍, but be inconsistent with POSIX (which I'm okay with, just noteworthy):

$ basename /
/

As for the typeclass: I think this will create more problems than it solves.

[joehillen]

As for the typeclass: I think this will create more problems than it solves.

Such as? I've never head of a typeclass creating problems.

[NorfairKing]

As for the typeclass: I think this will create more problems than it solves.

Such as? I've never head of a typeclass creating problems.

1. Error messages, now you'll get 'no instance of WhateverClassWeUseversusit is not a File`
2. Ambiguities: Type classes can counteract type inference in some cases.
3. Unnecessary complexity (in this case) because you mostly either deal with a file or a path, not both.

[harendra-kumar]

Making basename polymorphic is not really the core issue here, we can discuss that in a separate issue so that we do not digress. Let me answer the other question about parent "/" = "/". This is POSIX standard convention.

We are (rather incorrectly) calling the POSIX dirname as parent. If the meaning of parent were really parent then the parent of /x/y/z should have been the parent of /. What we mean instead is the POSIX dirname equivalent i.e. everything except the last component of /x/y/z which is /x/y.

If we use the same dirname intuition for /, then the dirname of / being / makes more sense. If we consider the full name of "/" as "/." instead (which is equivalent) then the dir part becomes / and the base part becomes ., fitting into our story. Though as @NorfairKing noted, making "." the basename of / is not the same as POSIX but that's because POSIX does not distinguish between Rel and Abs types and we have to!

isParentOf and parent are not really semantically related, if they were we could say isParentOf p x = p == parent x but that's not true. The misuse of the name parent is at the core of its inconsistency with isParentOf as well. We should rename isParentOf to isPrefixOf or something like that and it will become clearer that it need not be consistent with parent which in turn should be renamed to dirname.

[NorfairKing]

Making basename polymorphic is not really the core issue here, we can discuss that in a separate issue so that we do not digress. Let me answer the other question about parent "/" = "/". This is POSIX standard convention.

👍

The misuse of the name parent is at the core of its inconsistency with isParentOf as well.

Should we rename it? (let's make that a seperate issue, if so)

it will become clearer that it need not be consistent with parent

I disagree, it still needs to be consistent. Every parent is a prefix.

By the way, nothing says that we have to make the API look like the shell equivalent.

What do you suggest then, concretely and stepwise?

[harendra-kumar]

Should we rename it? (let's make that a seperate issue, if so)

isAncestorOf or isDirPrefixOf might convey the meaning more effectively. But let's defer it to another issue.

I disagree, it still needs to be consistent. Every parent is a prefix.

If that is desired we can fix isParentOf so that it treats "/" as a prefix of "/", the same way we treat "/" as parent of "/".

By the way, nothing says that we have to make the API look like the shell equivalent.

Yes, of course. But as long as there is no other serious conflict it is always helpful to reuse an established and popular API. One, that API has been through the test of time, reviewed and used by countless people. Two, it helps people who have gotten used to it, for example initially I always made the mistake of using dirname in the path package with the POSIX semantics only to later realize it is different.

What do you suggest then, concretely and stepwise?

To Fix #18:

1. Do not merge this PR (we retain the current parent "/" = "/" behavior). Drop Make parent and isParentOf consistent (fixes #33) #35 as well which is a similar PR pending merge.
2. merge Allow . as a relative path #34 (Allow . as a relative path)
3. Allow dirname "/" = ".", this completes the fix for Breaking the guarantees of 'Path Rel Dir' #18

Nice to have's, issues to be raised/discussed:

1. As discussed above, fixing parent and dirrename should return In MonadThrow m #33 is optional, if we want to, then we can just fix isParentOf for the root dir/drive case.
2. renaming of isParentOf
3. having a polymorphic basename

[NorfairKing]

I'm okay with this plan, but I want to hear others' opinion. (I thought @chrisdone didn't like this approach.)

[sjakobi]

I'm ok with keeping parent :: Path Abs t -> Path Abs Dir and parent "/" = "/". It's sad that so much work won't be merged but IMO it's better to half-heartedly not make a change than to make a half-hearted change.

I'm still rather -1 on dirname "/" = ".". With #34 we'd then also have dirname "." = "." which IMO marks a shift from a non-intuitive edge case to a non-intuitive design flaw.

[NorfairKing]

it's better to half-heartedly not make a change than to make a half-hearted change.

👍

I'm still rather -1 on

I'm wondering if it would be better to offer both API's?

[joehillen]

I'm wondering if it would be better to offer both API's?

I like that idea. It allows users to decide which behavior is desirable.

[harendra-kumar]

I'm still rather -1 on dirname "/" = ".". With #34 we'd then also have dirname "." = "." which IMO
marks a shift from a non-intuitive edge case to a non-intuitive design flaw.

One way to think about it is / being the root for absolute paths (AbsRoot) and . being the root for relative paths (RelRoot). You cannot go beyond the root in each case. The representation of the root has to be different because they are different types.

It does not sound intuitive because the representation "." is overloaded with two meanings i.e. the RelRoot as well as the identity of concatenation for relative paths. Conceptually if we write it like this, then it may look more intuitive:

dirname AbsRoot = Identity
dirname RelRoot = Identity

We can conclusively establish that it is a design flaw if we can find one practical problem that it causes and is against the accepted uses of "." and "/".

[mrkkrp]

Closed in favor of #34.