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

Remove system-filepath/system-fileio dependencies #248

Merged
merged 4 commits into from
Feb 3, 2018
Merged

Remove system-filepath/system-fileio dependencies #248

merged 4 commits into from
Feb 3, 2018

Conversation

PierreR
Copy link
Contributor

@PierreR PierreR commented Feb 1, 2018

Fix #239

@PierreR
Copy link
Contributor Author

PierreR commented Feb 1, 2018

This is a first WIP draft.

Please point me to obvious mistakes.
As discussed one is probably that System.FilePath.takeDirectory cannot be a substitute to Filesystem.Path.parent ?

Copy link
Collaborator

@Gabriella439 Gabriella439 left a comment

Choose a reason for hiding this comment

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

I wouldn't worry about the difference between System.FilePath.takeDirectory and Filesystem.Path.parent. It only matters if somebody tries to import a directory which won't be supported after I standardize the import semantics.

Specifically, see dhall-lang/dhall-lang#72 which indicates that
support for @ (and therefore, support for importing directories) will not be included in
the language standard.

@@ -411,11 +403,11 @@ canonicalize (File hasHome0 file0:paths0) =

-- `clean` will resolve internal @.@/@..@'s in @currPath@, but we still
-- need to manually handle @.@/@..@'s at the beginning of the path
combine url path = case Filesystem.stripPrefix ".." path of
combine url path = case List.stripPrefix ".." path of
Copy link
Collaborator

Choose a reason for hiding this comment

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

Data.List.stripPrefix behaves slightly differently from Filesystem.stripPrefix:

>>> Filesystem.Path.CurrentOS.stripPrefix ".." "../foo"
Just (FilePath "foo")
>>> Data.List.stripPrefix ".." "../foo"
Just "/foo"

I believe this should be `List.stripPrefix "../" instead

Just path' -> combine url' path'
where
url' = parentURL (removeAtFromURL url)
Nothing -> case Filesystem.stripPrefix "." path of
Nothing -> case List.stripPrefix "." path of
Copy link
Collaborator

Choose a reason for hiding this comment

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

Similarly, I believe this should be List.stripPrefix "./" instead

where
strip p = case Filesystem.stripPrefix "." p of
strip p = case List.stripPrefix "." p of
Copy link
Contributor Author

@PierreR PierreR Feb 2, 2018

Choose a reason for hiding this comment

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

Should I also change "." for "./" here ?

Copy link
Contributor Author

@PierreR PierreR Feb 2, 2018

Choose a reason for hiding this comment

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

Do we still need to call strip after normalise ? (tests are passing either way)

Copy link
Collaborator

Choose a reason for hiding this comment

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

You're right that we don't need stripPrefix here because System.FilePath.normalise removes any preceding ./s (unlike Filesystem.Path.collapse)

@PierreR
Copy link
Contributor Author

PierreR commented Feb 3, 2018

Let me know if you are expecting something more from this PR. I can rebase to avoid the conflict if it is more convenient on your side.

Copy link
Collaborator

@Gabriella439 Gabriella439 left a comment

Choose a reason for hiding this comment

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

Looks great! I'll go ahead and merge once CI is green

@Gabriella439 Gabriella439 merged commit ff555ed into dhall-lang:master Feb 3, 2018
@Gabriella439
Copy link
Collaborator

Thank you for contributing this :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants