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

WIP: Add function stripFilePath #75

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

fendor
Copy link

@fendor fendor commented Nov 12, 2019

Closes #73

Implementation draft to get the discussion started.

-- >>> stripFilePath "/app" "/app/Lib/File.hs"
-- Just "Lib/File.hs"
stripFilePath :: FilePath -> FilePath -> Maybe FilePath
stripFilePath "." fp
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about:

> stripFilePath "./." "lol"
Nothing
> stripFilePath "./" "lol"
Nothing

I can't figure out what semantics a user would expect here.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could take rust as a precedent?

stripPrefix("lol", "./.") -> Err(StripPrefixError(()))
stripPrefix("lol", "./") -> Err(StripPrefixError(()))

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, so do we want MonadThrow here maybe?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's in general a possible idea, for the whole API to have those for the case of errors.

--
-- >>> stripFilePath "/app" "/app/Lib/File.hs"
-- Just "Lib/File.hs"
stripFilePath :: FilePath -> FilePath -> Maybe FilePath
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My apologies, my implementation is very naive since I have never needed anything like that (I personally don't even know what windows file namespaces are)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure... I've implemented a filepath AST and parser here: #99

That would be more strict wrt special windows filepaths. I haven't decided yet how to use this AST yet... it's pretty annoying to pattern match on it. Using just lexemes on the other hand throws away some information. If you have ideas, let me know.

| isRelative fp = Just fp
| otherwise = Nothing
stripFilePath dir' fp'
| Just relativeFpParts <- splitDir `stripPrefix` splitFp = Just (joinPath relativeFpParts)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the splitpath -> joinpath idiom can sometimes turn an invalid filepath into a valid one, e.g.: #12 (comment)

| Just relativeFpParts <- splitDir `stripPrefix` splitFp = Just (joinPath relativeFpParts)
| otherwise = Nothing
where
dir = normalise dir'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a friend of ViewPatterns for this, unless there's a reason we can't use that here

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the whole implementation is a bit trash atm, so no reason other than trying to be as simple as possible

stripFilePath' (x:xs) (y:ys)
| x `equalFilePath` y = stripFilePath' xs ys
| otherwise = Nothing
stripFilePath' [] ys = Just ys
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

> stripFilePath "lol" "lol"
Just ""

I'm not sure whether we should allow this or not (most of the filepath API allows empty paths), but it at least warrants its own doctest line, because for some it might be unexpected.

This also means this function does not guarantee a valid filepath.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point!

BTW, rust has the same:

stripPrefix("lol", "lol") -> Ok("")

@hasufell hasufell added this to the 1.5 milestone Nov 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add function stripPrefix
2 participants