-
Notifications
You must be signed in to change notification settings - Fork 211
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
Fix import logic with --file for dhall-to-{json,yaml} #1191
Conversation
@@ -951,13 +953,18 @@ handleSpecialDoubles specialDoubleMode = | |||
codeToValue | |||
:: Conversion | |||
-> SpecialDoubleMode | |||
-> Text -- ^ Describe the input for the sake of error location. | |||
-> Maybe FilePath -- ^ The source file path. If no path is given, imports | |||
-- are resolved relative to the current directory. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@deepfire You introduced the Text
input description in dhall-lang/dhall-json#20. Is this change ok for you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this change is fine
dhall/src/Dhall/Import.hs
Outdated
|
||
-- | Resolve all imports within an expression, importing relative to the given | ||
-- directory. | ||
loadWithDir :: FilePath -> Expr Src Import -> IO (Expr Src X) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would loadWithRootDirectory
be a better name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe loadRelativeTo
?
resolvedExpression <- Dhall.Import.load parsedExpression | ||
let rootDirectory = case mFilePath of | ||
Nothing -> "." | ||
Just fp -> System.FilePath.takeDirectory fp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that I didn't adopt dhall
's way of interpreting --file -
as stdin
here, as I think it's a bit redundant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then we should probably change dhall
to match, for consistency
dhall/src/Dhall/Import.hs
Outdated
|
||
-- | Resolve all imports within an expression, importing relative to the given | ||
-- directory. | ||
loadWithDir :: FilePath -> Expr Src Import -> IO (Expr Src X) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe loadRelativeTo
?
@@ -951,13 +953,18 @@ handleSpecialDoubles specialDoubleMode = | |||
codeToValue | |||
:: Conversion | |||
-> SpecialDoubleMode | |||
-> Text -- ^ Describe the input for the sake of error location. | |||
-> Maybe FilePath -- ^ The source file path. If no path is given, imports | |||
-- are resolved relative to the current directory. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this change is fine
resolvedExpression <- Dhall.Import.load parsedExpression | ||
let rootDirectory = case mFilePath of | ||
Nothing -> "." | ||
Just fp -> System.FilePath.takeDirectory fp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then we should probably change dhall
to match, for consistency
AppVeyor fails due to
I'd suggest clearing the cache, but this might be about the artifact deployment to GitHub?! |
a8e9cde
to
502ac78
Compare
Fixes #1183.