-
Notifications
You must be signed in to change notification settings - Fork 409
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
Fancy generic templating stuff for nested data #620
base: master
Are you sure you want to change the base?
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
Closes jaspervdj#507 (actually was fixed by 9ec43a6 already, this just adds the test)
See jaspervdj#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`
@jaspervdj What do you think about 73cc078? This seemed like the right idea, but my understanding of |
I like what this PR tries to accomplish -- it's something I've also wanted for a long time.
No, I don't think we need to support this. In the worst case we can add escaping `"foo.bar" later if it turns out to be truly necessary.
I think it makes sense to do lexical scoping -- so the outer context should be sustained, but the inner context can overwrite it if there's a conflict. Going to have a look at the code now. |
-- | Turn on caching for a compilation value to avoid recomputing it | ||
-- on subsequent Hakyll runs. | ||
-- The storage key consists of the underlying identifier of the compiled | ||
-- ressource and the given 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.
Typo: resource
where | ||
mdRsc = fromFilePath $ flip addExtension "metadata" $ toFilePath identifier | ||
getResourceInfo :: FilePath -> Identifier -> IO (Identifier, ResourceInfo) | ||
getResourceInfo directory identifier = do |
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.
Do we need to support asking for resource info about .metadata
files?
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.
I guess I should rename this function to getFileInfo
. It is called on every (non-ignored) file in the directory, and then it decides itself whether that file is metadata of another resource or a resource itself.
rssTemplate :: String | ||
rssTemplate = T.unpack $ | ||
rssTemplate :: Item String | ||
rssTemplate = Item "templates/rss.xml" $ T.unpack $ |
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 we should call these something like Item "_hakyll/templates/rss.xml"
so they can't conflict with actual user-provided items?
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.
Ah, this change is actually part of #462 on which this PR is based on :-)
I think the Item
here is solely used for generating template error messages, it never faces the user anywhere. Or am I missing something that Compiler
internals would do to an Item
?
-- | Creates a variadic function field. | ||
-- | ||
-- The function will be called with the dynamically evaluated string arguments | ||
-- from the template as well as the page that is currently rendered. |
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.
Thanks for improving the docs!
, template | ||
, unTemplate | ||
, getOrigin |
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.
Do we need to export unTemplate
? In either case I'd call these templateElements
and templateOrigin
.
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.
I think we need to export them from Template.Internal
, yes. About renaming them: unTemplate
had been used and exported before as part of the non-smart Template
constructor.
(k:ks) | k == key -> lookupNestedValue ks (Item i val) | ||
_ -> failBranch $ "Tried field " ++ key -- and functionField get | ||
where | ||
get = let (h:rest) = key in "get" ++ toUpper h : rest |
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.
I don't really understand what this does 🤔
Are we encoding getFoo
as special keys?
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.
This allows one to create a dataField "foo" (Object metadata)
or something, then use $foo.bar.baz$
in templates or $getFoo("bar", "baz")$
- this allows lookup with dynamic names (generated by another field) and also supports names with dots (or any other special syntax).
I think the cleanest thing would probably to consider a |
Yes, that's what I've been trying to do with |
Yes, I figured that out when trying it - I need to have some control over the contexts in the loops. That means my current approach with |
@@ -92,7 +94,7 @@ data ContextField | |||
= EmptyField | |||
| StringField String | |||
| forall a. ListField (Context a) [Item a] | |||
|
|||
| forall a. LexicalListField (forall b. Context b -> a -> Context b) [a] |
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.
Lexical scoping was not possible with the current ListField
constructor, needed something more advanced :-) Also requires RankNTypes, not sure whether that's a bad thing.
Maybe it's also possible to further generalise this, but I don't see a way to do it without breaking compatibility for everyone who directly uses ListField
. Maybe these should be moved to a Context.Internal
module anyway.
module Hakyll.Web.Template.Context | ||
( ContextField (..) | ||
, Context (..) | ||
, context | ||
, functionContext |
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.
I wonder whether we should export the general functions under new names. I don't really like context
and functionContext
, as they construct contexts for specific keys. Should be keyContext
at best.
Do you think it would do any harm to just widen the type of field
and functionField
?
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.
I think it's fine to widen the type if it doesn't break existing code. I'm a bit afraid it could break existing code though, since people usually use {-# LANGUAGE OverloadedStrings #-}
.
To get back to your earlier question, there is no assumption that every Identifier has an underlying resource. In fact, it is quite common to produce additional pages based on just existing pages, or just Haskell code, so there is no resource matching the identifier name on the disk. |
but allow resources without a "body", consisting only of their metadata file (Even if that sounds a bit crazy)
Is there any news on this functionality? |
This takes various ideas from #529, #572 and #565 into action.
I've got a data file (json, or better yaml as that has comments) in my project whose contents I want to use in rendering various files - both html and json. Call it "shared (meta)data" if you want.
My first idea was a classic
Compiler
- parse the JSON file using Aeson, store in a snapshot, spit back out (without whitespace). Access the parsed data from the snapshot elsewhere (in the html templates).Problem: snapshotted data must be a
Binary
instance. Hakyll can do this already with data, but theBinaryYaml
(a newtype wrapper overValue
) is not exported.My second idea was using Hakyll's builtin metadata parsing. Have a
xyz.metadata
file, usegetMetadata "xyz"
in the Compiler monad. Hakyll would do the parsing and caching for me, right?Problem:
xyz
does not exist as a resource, so no metadata can be read from it (instead it always comes back as an empty object by default).I guess this could have been solved with an empty
xyz
file, but instead I went ahead andfixedupdated theProvider
to consider it as existing when there's axyz.metadata
file.Next step: write a custom context that allows me to access and iterate deep (nested)
Value
s - generically, without parsing the JSON in a dedicated structure and supplying the relevant list fields.Converting Aeson
Value
s into the respective strings was cumbersome though, and Hakyll already does havetoString
which I didn't want to replicate. Also this functionality seemed so useful that it might be interesting to add natively, possibly also forMetadata
like #572.I've got a proof of concept working in the attached commits (starting at "WIP", the rest is from #462), but this is much less a pull request than a request for comments as of now. What do you all think? Specifically
$for(…)$
loop that iterates an array, or an object? Should the outer context be sustained, i.e. if$var$
is available outside of the loop should it be accessible from inside the loop as well?