Introduce --overwrite-files flag #1322
Conversation
bolt12
left a comment
bolt12
left a comment
There was a problem hiding this comment.
collect all files that need to be written and then decide if we can overwrite them or not. This approach also plays well with the create-dirs flag.
dschrempf
left a comment
dschrempf
left a comment
There was a problem hiding this comment.
Thanks Armando! I had a look, because I am also working on the Artefacts at the moment, and left a few comments! It will be great to have this in!
|
I have successfully implemented the solution where if there are any files already present, then the command is going to fail without written anything! Reviews are welcome! |
dschrempf
left a comment
dschrempf
left a comment
There was a problem hiding this comment.
Thanks! Sorry for my nitty reviews, I have become fond of the Artefact module...
I think the "pending write" idea is a great one, I am just unsure if it has the desired effect (write all files, or none) in the way it is implemented at the moment.
I think we must have a "file system operation" storage in the artefact monad. Only at the end of the execution, when no Error trace has happened, these file system operations are executed one after the other.
Address review feedback Collect all write effects
dschrempf
left a comment
dschrempf
left a comment
There was a problem hiding this comment.
Thank you, it will be great when we have this in main! I left some more comments/questions, mostly about two aspects:
- Configuration (should we have this in
BackendConfig?); do we need to parse the options in all client commands (could be a separate ticket, I think). - I suggest moving the write actions into
Artefact.hs, but you may have reasons why you did not do that in the first place.
Cheers!
| bindgenConfig = | ||
| toBindgenConfig | ||
| (toFilePath packageRoot <$> config) | ||
| DoNotCreateDirStructure |
There was a problem hiding this comment.
Since you define conservative defaults anyway, we could use the defaults here, and move the comments to the definitions of the Default instances?
| data BackendConfig = BackendConfig { | ||
| backendTranslationConfig :: TranslationConfig | ||
| , backendHaddockConfig :: HaddockConfig | ||
| , backendOutputDirPolicy :: OutputDirPolicy |
There was a problem hiding this comment.
I think it is OK to have the policies in the backend configuration. On the other hand, then they have to be provided even when we do not create output files (e.g., TH mode).
We could also provide them with the respective artefacts, but then we have to do so for every artefact involving file output. What do you think?
There was a problem hiding this comment.
(This would have the advantage that we do not have to provide values to toBindgenCOnfig).
|
|
||
| -- | Content to be written to a file | ||
| -- | ||
| -- If it's TextContent then, depending on the FileOverwritePolicy, then this |
There was a problem hiding this comment.
Comment needs to be fixed.
| -> Artefact () | ||
| writeByCategory what hsOutputDir moduleBaseName = | ||
| mapM_ (uncurry writeCategory) . Map.toList . unByCategory | ||
| Foldable.foldl' (>>) (pure ()) . map (uncurry writeCategory) . Map.toList . unByCategory |
There was a problem hiding this comment.
Is there a reason why you have removed mapM_? I think it is nicer than a fold.
| input <- strArgument $ metavar "IN" | ||
| output <- strArgument $ metavar "OUT" | ||
|
|
||
| outputDirPolicy <- parseOutputDirPolicy |
There was a problem hiding this comment.
I think this should not be a command line option for literate mode. I am unsure how literate mode works in detail, but I think GHC uses a new tmp dir, so if this is overwriting something, it may be a bug. I would try using the conservative options here.
| <*> parseBaseModuleName | ||
| <*> optional parseOutput' | ||
| <*> parseInputs | ||
| <*> parseOutputDirPolicy |
There was a problem hiding this comment.
I think we should also go over these client commands (IncludeGraph, UseDeclGraph, Frontend etc.). Do they actually use the command line options at all, or is it better if we just use default values (see literate mode below). Of course, this could be a separate ticket and PR, just saying.
I just want to avoid that we offer command line options for things we don't even use in the command.
| artefact = runReaderT (runExceptT (runArtefact artefact)) env | ||
| artefact = do | ||
| (result, _, actions) <- runRWST (runExceptT (runArtefact artefact)) env () | ||
| return (result, actions) |
There was a problem hiding this comment.
Is there a reason why you return the actions and not perform them here?
I have the feeling they should be performed here. This also makes sense in that the comment at FileContent describes what the file operations do, but the code is not in the module but in HsBindgen.hs.
Also, I mentioned this already but I think we didn't discuss it properly yet: Why do we use an exception instead of an error trace? To my knowledge, we should avoid panics if possible and think about hs-bindgen as if it were a compiler.
The Bind operation of Artefact checks for error traces, and we could use that machinery and just emit an error trace.
| _ -> pure mbp | ||
| ) Nothing actions | ||
| case outputDirPolicy of | ||
| DoNotCreateDirStructure |
There was a problem hiding this comment.
I thought much less about this than you, and there may be subtleties involved I
do not see; but I have two remarks:
-
Could we do this in reverse order? Only check all dirs and paths if we configured to protect them, respectively.
-
Doesn't the right fold always traverse the complete list of actions, even if it finds a conflicting file early on? I think it is enough to stop at the first existing directory.
This closes #1117