Add various functions explicitly taking a root directory. #508
Conversation
|
One more wrinkle: the |
src/Dhall/Import/Types.hs
Outdated
| -- | State threaded throughout the import process | ||
| data Status = Status | ||
| { _stack :: [Import] | ||
| { _root :: FilePath |
There was a problem hiding this comment.
You don't need to add a _root field. Instead, you can make _stack a NonEmpty list of Imports and set the first one to the directory with a file named "."
| @since 1.6 | ||
| -} | ||
| inputDirFromWith |
There was a problem hiding this comment.
Are you sure that you want this function to take two FilePath arguments. Couldn't you infer the starting directory from the path of the source file?
There was a problem hiding this comment.
That's not guaranteed to be a path to the file; e.g., it's also <input> or <stdin> if that's where the data is actually coming from.
There was a problem hiding this comment.
Yeah, and also thinking more about your answer in the other issue I agree that it's necessary to disentangle the two concepts, so don't worry about this after all
|
OK, I removed |
src/Dhall/Import.hs
Outdated
| -- need to not display the first (innermost) import, and the | ||
| -- final (outermost) import is fake to establish the base | ||
| -- directory. Also, we need outermost-first. | ||
| toDisplay = drop 1 (reverse (drop 1 canonical)) |
There was a problem hiding this comment.
The doctest error appears to be due to the new drop 1 here. According to the comment this appears to be intentional, but are you sure that the innermost import is already displayed by e?
There was a problem hiding this comment.
I thought it was! :( I'm having a look now too. That reasoning was based on the previous code having a drop 1 in it but I might have confused myself while trying to get this function to work.
There was a problem hiding this comment.
Oh, it was on the wrong side of the reverse anyway for that. Now I don't know why the previous code had the drop 1 in it at all.
There was a problem hiding this comment.
@quasicomputational: I think the reason it had the other drop 1 is because it would implicitly insert ./. as the root of the the chain of imports before your change and the drop 1 removed it so it wouldn't show up in the chain of imports
|
Oh, I think I understand what was going on: the previous code was injecting its own fake import and that had to be dropped. We're still doing that, just more systematically. |
Now the importing code is not tied to the current working directory: it can resolve imports relative to any directory passed to it. Fixes dhall-lang#507.
Imports resolve directories relative to their parent import. Hence, we can put in a fake import at the root to control the resolution of relative imports.
My understanding of the previous code was wrong: it was dropping the outermost import because `canonicalizeImport` was already using the fake import trick.
quasicomputational
force-pushed
the
cwd-independence
branch
from
dd2f57e to
05b9629
Compare
src/Dhall.hs
Outdated
| to, a file to mention in errors as the source, a custom typing | ||
| context, and a custom normalization process. | ||
| @since 1.6 |
There was a problem hiding this comment.
Did you mean @since 1.15 or @since 1.16?
There was a problem hiding this comment.
Oops, yes. Might be contagious!
|
Thanks a bunch! |
|
You're welcome! Thank you, too 🙂 |
Now the importing code is not tied to the current working directory:
it can resolve imports relative to any directory passed to it.
Fixes #507.
Notes: I haven't added every flavour of
Dirfunction that I could have: I only put in the ones relevant to dhall-to-cabal, besides the most general one of each family that contains the actual implementation. These functions are getting a bit dense and we might consider some kind of alternative strategy here (e.g., anInputSettingstype, where today'sinputbecomes equivalent toinput defaultInputSettings).I didn't add an
importFilefunction as suggested in #507 and previously implemented in #467.I put the root directory in the
Statusobject because caching relative imports with different root directories isn't safe. However, I note that it should be immutable, which is currently only enforced by convention: theStateTwould allow modification of the root directory, despite that never being the correct thing to do.