Better error messages #722
Conversation
Force templates to be consumed completely, propagate error appropriately
...makes better error messages :-) Notice the breaking change in 'applyElem', where $if(...)$ conditions in templates now will throw errors if their field 'fail'ed (instead of just being 'empty')!
...when a more important error prevails. Also not throwing from 'if' conditions any more, only logging those errors to the debug screen
* boolFields used outside of 'if'-conditions now get a "stack trace" using a new 'NoField' they don't have to rely on 'error' any more * templates applied to their own file get proper description (did use incompatible paths/identifiers before) * renamed 'compilerFail' to more descriptive name
See #462 (comment) and below for detailed explanation Also abstracted out `testCompilerError` in the test suite, and added a `compilerTry` that is much easier to use (and specifically, to branch on) than `compilerCatch`
|
@bergus I'm pretty keen to merge this in soon. Do you think you can give it another review? I made a few changes to your code -- most of it just a number of renames, but I also changed the new error type to use |
|
Sure, I'll have a look. Haven't worked with Hakyll for some time now, though a project relying on #620 is still in my backlog... |
| @@ -132,7 +135,7 @@ data CompilerResult a | |||
| = CompilerDone a CompilerWrite | |||
| | CompilerSnapshot Snapshot (Compiler a) | |||
| | CompilerRequire (Identifier, Snapshot) (Compiler a) | |||
| | CompilerError (Reason [String]) | |||
| | CompilerError (CompilerErrors String) | |||
There was a problem hiding this comment.
This is why I did use the name Reason, (Compiler)Errors looks just weird here.
| | NoCompilationResult a | ||
| data CompilerErrors a | ||
| -- | One or more exceptions occured during compilation | ||
| = CompilationFailure (NonEmpty a) |
There was a problem hiding this comment.
What's your specific reasoning to avoid empty lists here? And why did compilerThrow change to produce a CompilationNoResult if no reason value is given?
(Not assuming it's a bad thing, I just don't understand the idea behind it).
There was a problem hiding this comment.
Also seems to make the Alternative.<|> implementation more complicated
There was a problem hiding this comment.
It's to prevent us from shooting ourselves in the foot -- if there's no error, we shouldn't get into a CompilationFailure scenario. This is not true for the other case where we have empty.
There was a problem hiding this comment.
After familiarising myself with the code again, I would heavily prefer
-- | Put a 'CompilerError' with multiple error messages as 'CompilationFailure'
compilerThrow :: NonEmpty String -> Compiler a
compilerThrow = compilerResult . CompilerError . CompilationFailureThe compilerThrow function is an internal function (only available on Compiler.Internal) so we can easily "break" it by changing the type. (IIRC it was earlier used in Context or so for transforming errors, but has been replaced there with more sensible alternatives). It never was meant to be called with an empty list, and neither in fail nor in <|> that could happen.
By having compilerThrow :: NonEmpty String -> Compiler a, we don't need to convert back and forth between lists and non-empty lists in <|>.
Where it does matter, and where I agree with you that we should prevent anyone from shooting themselves in the foot, is throwError in the MonadError [String] instance. Let's do
throwError errs = maybe (compilerNoResult errs) compilerThrow $ NonEmpty.nonEmpty errs
lib/Hakyll/Web/Feed.hs
Outdated
| renderFeed :: String -- ^ Default feed template | ||
| -> String -- ^ Default item template | ||
| renderFeed :: Item String -- ^ Default feed template | ||
| -> Item String -- ^ Default item template |
There was a problem hiding this comment.
This is my remaining nitpick about the PR -- changing this breaks backwards-compatibility, which is fine, since this is a relatively rarely used function so it's worth it. However, if we do change it, we should change it to the "right" type which is Template or Item Template rather than Item String.
There was a problem hiding this comment.
Template is fine, but it shouldn't be a template constructed by readTemplate which doesn't know about the origin of the template file (it just assumes a string literal), which is important for error messages. Doing the compileTemplateItem did allow me to use the Items path for the template, which I had explicitly added, without having to use any of the Template.Internal stuff to construct this "embedded Template".
| let source = T.unpack $ T.decodeUtf8 $(embedFile filePath) in | ||
| case parseTemplateElemsFile filePath source of | ||
| Left err -> error err | ||
| Right tpl -> template filePath tpl |] |
There was a problem hiding this comment.
Ah, this is how to do it properly with Template Haskell - thanks!
This resumes the excellent work done in #462