purescript-contrib / purescript-pathy

A type-safe abstraction for platform-independent file system paths.
Apache License 2.0
33 stars 17 forks source link

Some tweaks and updates for the next release #18

Closed garyb closed 8 years ago

garyb commented 8 years ago

Resolves #14, will resolve #5.

Wanted to get a few things in before we tag a new version :smile:

garyb commented 8 years ago

Ok, this is ready to review now. As well as fixing the linked issues and adding some new basic instances there's a new function:

-- | Marks an absolute path as sandboxed without sandboxing to a particular
-- | location.
sandboxAbs :: forall b. Path Abs b Unsandboxed -> Path Abs b Sandboxed

as we end up using a coercion to that effect all the time. I did consider making it Path a b Unsandboxed -> Path Abs b Sandboxed so it could turn any path into an absolute sandboxed path, but I'm not sure if that's a bit more questionable?

Also updated the formatting to be more consistent with how we do it everywhere else.

Also fixed some links in the docs to a non-existant examples file.

jdegoes commented 8 years ago

@garyb That function is not safe and defeats the point of sandboxing. You can change the signature to return Maybe (Path Abs b Sandboxed) and then return Nothing in the case where the parent path node (..) appears in the path.

garyb commented 8 years ago

@jdegoes you mean the version of sandboxAbs that works with any path? The current version doesn't have that problem, as you can't do that in an Abs path, can you?

If so, yeah, that's why I said it was questionable. I was going to handle Rel cases by doing something like prelativeTorootDir with a fromMaybe rootDir, etc. but it's already possible to easily do that without resorting to hacks, but making an Abs path sandboxed and still Abs is pretty tedious currently.

jdegoes commented 8 years ago

you mean the version of sandboxAbs that works with any path? The current version doesn't have that problem, as you can't do that in an Abs path, can you?

An absolute path may contain relative entries. Meaning that the signature of sandboxAbs is incorrect, in the sense you can produce a "sandboxed" path which attempts to access a directory higher than the root directory (which does not make any sense).

garyb commented 8 years ago

Ah ok, I see now.

garyb commented 8 years ago

I've removed the offending sandboxAbs.

I think fixing #5 should fix slamdata/slamdata#928 since that seems to be arising due to dir "." not being escaped, so when it is printed and re-parsed it is read as currentDir.

garyb commented 8 years ago

We'll have to release this as v2.0.0 as it didn't make the v1.0.0 cut - making the left/right stuff consistent means the API has a breaking change. Possibly changing things to escape properly could be considered a breaking change too, given the behaviour above with dir ".".

garyb commented 8 years ago

Any objections to me merging this as it is @jdegoes?

jdegoes commented 8 years ago

Looks good. 👍