Skip to content
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

Open
wants to merge 21 commits into
base: master
Choose a base branch
from

Conversation

bergus
Copy link
Contributor

@bergus bergus commented Mar 17, 2018

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 the BinaryYaml (a newtype wrapper over Value) is not exported.

My second idea was using Hakyll's builtin metadata parsing. Have a xyz.metadata file, use getMetadata "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 and fixed updated the Provider to consider it as existing when there's a xyz.metadata file.

Next step: write a custom context that allows me to access and iterate deep (nested) Values - generically, without parsing the JSON in a dedicated structure and supplying the relevant list fields.
Converting Aeson Values into the respective strings was cumbersome though, and Hakyll already does have toString which I didn't want to replicate. Also this functionality seemed so useful that it might be interesting to add natively, possibly also for Metadata 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

  • do we need to support keys that contains dots? If yes, how - and does it need to be backwards-comptible?
  • what fields should be available inside a $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?

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`
@bergus
Copy link
Contributor Author

bergus commented Mar 17, 2018

@jaspervdj What do you think about 73cc078? This seemed like the right idea, but my understanding of Provider is too shallow yet. Specifically, treating identifiers as resources just because a .metadata file exists for them might have some weird side effects - no file at that path exists, but resourceExists says otherwise. Also it does get matched by wildcard patterns that do not contain a file extension, and trying to compile such a non-existing file will fail.
Is my commit an appropriate hack, and the errors it could cause are warranted?
Or does this need a more holistic design approach - I also noticed there's still a TODO in the code about modification dates of metadata (in general, not only external files). Should metadata and contents (bodies) be versioned separately, as they are already cached separately?

@jaspervdj
Copy link
Owner

I like what this PR tries to accomplish -- it's something I've also wanted for a long time.

  • do we need to support keys that contains dots? If yes, how - and does it need to be backwards-comptible?

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.

  • what fields should be available inside a $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?

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.
Copy link
Owner

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
Copy link
Owner

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?

Copy link
Contributor Author

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 $
Copy link
Owner

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?

Copy link
Contributor Author

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.
Copy link
Owner

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
Copy link
Owner

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.

Copy link
Contributor Author

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
Copy link
Owner

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?

Copy link
Contributor Author

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).

@jaspervdj
Copy link
Owner

I think the cleanest thing would probably to consider a .metadata file and the resource as "one thing".

@bergus
Copy link
Contributor Author

bergus commented Mar 18, 2018

I think the cleanest thing would probably to consider a .metadata file and the resource as "one thing".

Yes, that's what I've been trying to do with M.fromListWith combine <$> mapM (getResourceInfo directory) list and the combine function.
The problem is that I don't know which code in Hakyll does depend on the assumption that every "thing" (Identifier) has a resource.

@bergus
Copy link
Contributor Author

bergus commented Mar 18, 2018

I think it makes sense to do lexical scoping

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 Context Value, Context (Int, Value) and Context (Text, Value) won't work, I need to pass the value to the creation of the context itself not extract it from the Item.

@@ -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]
Copy link
Contributor Author

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
Copy link
Contributor Author

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?

Copy link
Owner

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 #-}.

@jaspervdj
Copy link
Owner

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.

@nicola-gigante
Copy link

Is there any news on this functionality?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants