Enable translation of maps into directory trees #2448
Conversation
|
Thought. It might make sense to add a check on paths not being absolute and not containing Opinions? |
|
I went ahead and added the according security checks. |
|
@nikita-volkov I do have a pending PR #2437 with another possibility of constructing a directory tree and I am interested in you feedback there. Would it help you with your use case? |
|
@mmhat I've taken a look at the usage example provided in your PR. It looks like your solution provides a finer grain control, which should definitely find use in non-trivial tasks. However it also comes at a price of a more complicated API for the developer. I think both solutions should be merged. Mine will be useful for simple cases, providing a simpler API, yours will be useful for advanced cases. |
@nikita-volkov Thanks for the feedback; I think its a fair assessment! I also don't see a problem to support both ways and I'd be happy to see it merged. |
There was a problem hiding this comment.
I will let @nikita-volkov and @mmhat work out how to resolve any merge conflicts that might arise
|
Trying to resolve the conflicts after the changes by @mmhat. The only issue is with the added |
|
Anyway, I've adapted the changes. Now setting |
|
Great, I'll have another look tomorrow 👍 |
|
@nikita-volkov I had a look at your changes and added the CLI flags in nikita-volkov#1. This is a PR against your feature branch; Please review the changes there and give me some feedback. Regarding path separators in file names: I like the default (do not allow them) as it is now; The directory structure should primarily stem from the Dhall code and not from the content of an opaque text value. |
|
@mmhat I don't like the idea of parameterising the compiler with various modes of interpretation of one codebase. This will likely lead to all sorts of confusion for the users and will require authors to inform the users about the compiler settings needed to compile their codebase. That's the reason I'm skeptical about the added There can only be one truth and IMO it's best for build tools to exhibit the same property: there can only be one interpretation of one codebase. That's what makes builds predictable. Pinging @Gabriella439 to draw some attention. |
|
I don't really disagree with you on the fact that it is a good thing to have one and only one interpretation of a codebase. My point is more about the defaults here: I see all three checks as safety features guarding against programming errors (e.g. no path separators) or security risks (e.g. no ".."). The CLI flags are simply escape hatches in the case You Know What You Are Doing. So to me its more about disabling safety checks than interpreting the program in a different way. If we disagree here on the defaults of those settings then I am happy to discuss that, but I'd like a switch for enabling/disabling anyway. |
There was a problem hiding this comment.
I personally think it's fine to add the additional options that @mmhat suggested. I'd be more concerned if the options changed the shape of the expected directory tree type, but that's not the case here
|
|
||
| toDirectoryTree allowSeparators (path </> Text.unpack key) value | ||
| case keyPathSegments of | ||
| -- Fail if path is absolute, which is a security risk. |
There was a problem hiding this comment.
Perhaps use System.FilePath.isRelative?
| (dirPath, fileName) <- case reverse keyPathSegments of | ||
| h : t -> | ||
| return | ||
| ( Foldable.foldl' (</>) path (reverse t) | ||
| , h ) | ||
| _ -> | ||
| die | ||
|
|
||
| Directory.createDirectoryIfMissing True dirPath | ||
|
|
||
| toDirectoryTree allowSeparators (dirPath </> fileName) value |
There was a problem hiding this comment.
I think you want something like this:
| (dirPath, fileName) <- case reverse keyPathSegments of | |
| h : t -> | |
| return | |
| ( Foldable.foldl' (</>) path (reverse t) | |
| , h ) | |
| _ -> | |
| die | |
| Directory.createDirectoryIfMissing True dirPath | |
| toDirectoryTree allowSeparators (dirPath </> fileName) value | |
| (dirPath', fileName) <- case reverse keyPathSegments of | |
| h : t -> | |
| return | |
| ( Foldable.foldl' (</>) path (reverse t) | |
| , h ) | |
| _ -> | |
| die | |
| let dirPath = path </> dirPath' | |
| Directory.createDirectoryIfMissing True dirPath | |
| toDirectoryTree allowSeparators (dirPath </> fileName) value |
In other words, dirPath needs to incorporate path as a prefix, otherwise the Map fields will be created as subdirectories relative to PWD when they should be relative to path.
Note that nikita-volkov#1 fixes this, too
Resolves #2460