Add NormalizedFilePath from ghcide

[jneira]

• To track in types we have a normalized path and it is sure to conver to normalized uri without an extra normalization step
• Fixes Refactor file uris to make possible skip normalization #212 and let ghcide not worry about its specofoc implementation
• The implementation only calls FP.normalise once in fromNormalizedFilePath . uriToNormalizedFilePath . normalizedFilePathToUri . toNormalizedFilePath, like the actual ghcide impl so i hope the performance will be not degraded
  • it would be useful to have a benchmark over it though
• Significant changes:
  • platform* functions are not public any more cause they are a implementation detail that should not be exposed
  • the hash of NormalizedFilePath is equal now to the hash of NormalizedUri: imo they must match for all cases and we avoid an additional hash call (/cc @cocreature)
  • tests dont check both os's anymore so the actual ci will not tests windows uris. I want to add windows ci soon so the tests will be recoved
    • The test suit passes locally in my windows systems
  • uriTofilePath and filePathToUri are now deprecated, with the goal of replace them with the new NormalizedFilePath functions. They should be removed in the next breaking release after this one.
• Caveats: i've kept the ghcide corner case for empty file paths but i think it would be nice to not include it in the library. Not sure if it is possible without significant changes in the lib or ghcide code

[alanz]

LGTM, would want sign-off from @cocreature and/or @mpickering

[cocreature]

The changes look reasonable but I would prefer to keep the way the tests are currently setup so that you can test Windows filepath conversions without access to a Windows machine. Yes, setting up CI would catch this but it’s much more convenient to be able to test this locally.

[jneira]

@cocreature ok, i would prefer to keep the platform* functions private cause imo they should not be used out of the module but i'll add them to exports labeling them as "internal" in comments.

To have CI running in windows have advantages beyond this concrete module. Not sure if you have tried to add windows to ghcide azure ci but i would recommend it strongly 😃

[jneira]

@cocreature ok, i've recovered the tests over the platform aware functions as they was in 2dceca9, cause those functions had been reverted to that version (they are not doing a full normalization now).
Anyway i've added a warning to platform aware functions to make clear they should not be used outside the package.
Otoh i've removed the deprecations over uriToFilePath and filePathToUri cause Uri is being used and it is the one that has the Aeson instances, so replace it is a long term task.

[jneira]

@cocreature what do you think about remove this corner case? i would prefer to keep it in ghcide instead the lib

[jneira]

@cocreature i think this part is making roundtrip tests fail and i am not sure it is intentional. Consider the repl session:

> toNormalizedFilePath "C:\\a#s"
NormalizedFilePath "C:\\a#s"
> normalizedFilePathToUri $ toNormalizedFilePath "C:\\a#s"
NormalizedUri 1827512744 "file:///C:/a#s"
> uriToNormalizedFilePath $ normalizedFilePathToUri $ toNormalizedFilePath "C:\\a#s"
Just NormalizedFilePath "C:\\a"

shouldnt NormalizedUri _"file:///C:/a#s" be NormalizedUri _"file:///C:/a%23s"?
Like the old functions already does:

> filePathToUri "C:\\a#s"
Uri {getUri = "file:///C:/a%23s"}
> uriToFilePath $ filePathToUri "C:\\a#s"
Just "C:\\a#s"

[jneira]

I've removed temporary the extra unescape/escape uri step to check if tests pass

[jneira]

I've added some changes to make the roundtrip tests pass, to do so i've have to change the function that normalizes the percent encoding

I want to highlight the new test cases:

it "converts a file path with reserved uri chars to a normalized URI and back" $ do
    let start = if isWindows then "C:\\" else "/"
    let fp = start ++ "path;part#fragmen?param=val"
    let nuri = toNormalizedUri (filePathToUri fp)
    uriToFilePath (fromNormalizedUri nuri) `shouldBe` Just fp

 it "converts a file path with substrings that looks like uri escaped chars and back" $ do
    let start = if isWindows then "C:\\" else "/"
    let fp = start ++ "ca%C3%B1a"
    let nuri = toNormalizedUri (filePathToUri fp)
    uriToFilePath (fromNormalizedUri nuri) `shouldBe` Just fp

Before changing the normalization of percent encoding those tests did not hold and files with #, ?, etc was not properly handled.

To achieve that i had to add another computation to toNormalizedFilePath: stripPrefix "file://".
However i've removed the T.pack $ escapeURIString isUnescapedInURI $ unEscapeString $ T.unpack t from toNormalizedFilePath so afaiu the performance of ghcide (that uses the last one) should be even better

[cocreature]

LGTM, thanks a lot!

[jneira]

@cocreature sorry for make more changes after your review, but i have a wip pr to adapt ghcide to this changes and i think it worths move the empty path case to ghcide cause other downstream packages could have troubles with that corner case.
The pr is here: https://github.com/digital-asset/ghcide/compare/master...jneira:norm-filepath?expand=1

[alanz]

@jneira when you and @cocreature are happy with this please merge it.

I think we should make a new haskell-lsp release soon, there have been a number of changes recently.

[jneira]

In the light of haskell/ghcide#479 i think this pr could be merged as is. I'll do tomorrow if nobody disagree