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

text/template: make blocks easier to use #14285

Closed
bep opened this issue Feb 10, 2016 · 23 comments
Closed

text/template: make blocks easier to use #14285

bep opened this issue Feb 10, 2016 · 23 comments
Milestone

Comments

@bep
Copy link
Contributor

@bep bep commented Feb 10, 2016

I spent a little too much time trying to get the new block syntax in Go 1.6 implemented in Hugo, with no success.

Which is telling me that you should try to supply a "more real" example in the documentation.

Hugo uses the built-in associative transitive map to store its templates, so adding a new template is something like:

hugoTemplates.New("homePage").Parse(someTemplate)

As I understand the new block implementation, one needs to first parse it with the master file (with the block keywords), then do a "overlay" (a modification, a re-parsing) of the same template with the template with the definitions.

How to do that with the above I cannot figure out (other than the simple standalone clone example in the doc -- I could clone it, but how do I replace the template in the map?).

My naive approach would be:

m, _ := hugoTemplates.New("home").Parse(master)
m.Parse(overlay)

And this works ... for the first template. Adding a second breaks in confusing ways.

Ref. gohugoio/hugo#1832

Also see #3812

@bep bep changed the title text/template: A more complete block example text/template: Add a more complete block example Feb 10, 2016
@ianlancetaylor ianlancetaylor added this to the Go1.7 milestone Feb 10, 2016
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Feb 10, 2016

If you want to a question about the new syntax, please see https://golang.org/wiki/Questions . Thanks.

@bep
Copy link
Contributor Author

@bep bep commented Feb 10, 2016

@ianlancetaylor -- no, this was plain and simple issue report. If something was unclear, don't hesitate to ask.

I'm not sure if this is a bug in the template package or simply some missing documentation, though.

@bep bep changed the title text/template: Add a more complete block example text/template: Template blocks only works in isolation Feb 10, 2016
@adg
Copy link
Contributor

@adg adg commented Feb 11, 2016

I think the issue is that hugo does things one way, while the block feature wants to do them another. The deal with blocks is that you create new *template.Template values for each "version" of the master templates.

Maybe @spf13 can comment on the approach taken by Hugo.

I am working on a better example of template blocks.

@bep
Copy link
Contributor Author

@bep bep commented Feb 11, 2016

Hugo's approach is basically:

root := template.New("")
root.Funcs(funcs)
// add embedded templates ...

// Then add all the user and theme templates (including partials, shortcodes, views)
// may use a master/overlay construct
root.New("template1").Parse(...)
root.New("template2").Parse(...)

// ...

templateForPage := root.Lookup("template1")

If I read you correct, this approach will not work with blocks and master/overlay templates, but you would have to get rid of the global and track the individual templates (with funcs and all the other dependencies (templates) duplicated/cloned) in your own registry?

I guess that would work, but it sounds like fighting the original design .

Not sure what I expected by the new block feature, but I did expect it to be more integrated, something a la:

root.New("template1").Parse(master, overlay)
@spf13
Copy link
Contributor

@spf13 spf13 commented Feb 11, 2016

@bep knows this as well as anyone.

I'm not sure if I understand this all correctly at all. From my reading of this thread and some of the related ones it sounds to me like go template blocks will use a vanilla *template.Template for the children (overriders)?

If so I think the approach the go team has taken makes sense from an implementation perspective, but is also a bit shortsighted. I don't think it's reasonable to assume that people would want a plain *template.Template for each of the master templates, particularly if they (as is in Hugo's case) have extended the functionality of the templates with additional functions. It seems very odd to me that these functions would be present in the parent but not in the children templates (not sure what the right terminology is here).

I've always thought the templates worked a bit strangely as you need to create a template and then chain off of it to create others, but it's worked really well for Hugo by creating an empty template at the root, customizing it and then all of the others effectively extend it. It's disappointing that this feature would not respect that valid use pattern.

Please correct me if I'm wrong.

@gopherbot
Copy link

@gopherbot gopherbot commented Feb 12, 2016

CL https://golang.org/cl/19443 mentions this issue.

@adg
Copy link
Contributor

@adg adg commented Feb 12, 2016

The CL that @gopherbot just mentioned is my attempt to demonstrate the intended use of block.

@adg
Copy link
Contributor

@adg adg commented Feb 12, 2016

@spf13 this sentence

I don't think it's reasonable to assume that people would want a plain *template.Template for each of the master templates

leads me to believe you misunderstand how blocks should be used. You first declare a master *template.Template and then Clone that value to parse in new templates to replace blocks inside the master. Take a look at the above CL and let me know whether that makes sense.

@bep
Copy link
Contributor Author

@bep bep commented Feb 12, 2016

If I read you correct, this approach will not work with blocks and master/overlay templates, but you would have to get rid of the global and track the individual templates (with funcs and all the other dependencies (templates) duplicated/cloned) in your own registry?

@adg: I guess the CL was a "yes" to the question above.

I suggest you keep this issue open to track obvious improvements to the "master/overlay" feature.

I will put the Hugo-issue in freeze-mode for now.

@adg
Copy link
Contributor

@adg adg commented Feb 12, 2016

@bep yes.

@adg
Copy link
Contributor

@adg adg commented Feb 15, 2016

I'm not sure that there's anything to do wrt this issue. Anyone object to my closing it?

@bep
Copy link
Contributor Author

@bep bep commented Feb 15, 2016

I object.

Your new example is just a little slightly more practical example of the same.

The block feature is half-finished.

If you look at template.Parse, there is plenty of logic to keep all the associated templates in the same namespace in sync.

With the current master/overlay-dance you will have to re-implement similar logic yourself, which may or may not be a simple thing, depending on the use case.

So, a template.Overlay method or similar would be good. This is a very common use case and deserves better support from Go's std-lib.

Simplicity is hard.

@adg
Copy link
Contributor

@adg adg commented Feb 15, 2016

Right. We considered Overlay in the original design, but thought that what is in 1.6 is a good incremental step.

This issue is [now] tracking API improvements that make blocks more useful. Apologies if I misinterpreted the original intent of the issue.

@adg adg changed the title text/template: Template blocks only works in isolation text/template: make blocks easier to use Feb 15, 2016
@adg adg reopened this Feb 17, 2016
@adg
Copy link
Contributor

@adg adg commented Feb 24, 2016

@moorereason
Copy link

@moorereason moorereason commented Feb 24, 2016

That would be CL https://golang.org/cl/19788

@adg
Copy link
Contributor

@adg adg commented Feb 24, 2016

Yeah, I was hoping @gopherbot would chime in sooner!

@gopherbot
Copy link

@gopherbot gopherbot commented Feb 24, 2016

CL https://golang.org/cl/19788 mentions this issue.

@bep
Copy link
Contributor Author

@bep bep commented Feb 24, 2016

@agl what you have added is convenience methods that behaves very differently from their Parse and ParseFiles counterparts.

I'm sorry if I couldn't convey my message better, but these two methods I think are better left out of the API as is, it will confuse more than it helps.

What I would expect of template.Overlay if I hadn't read the GoDoc is something ala this pseudo code:

master := template.New("master").Funcs(funcs).Parse("some template")

master.Overlay("overlay").Parse("some other template")

// then later, somewhere else

overlay := master.Lookup("overlay")

This is how the template API looks like. There may be technical challenges making this overlay construct fit into that API, but if it's impossible, then just leave it as is.

@adg
Copy link
Contributor

@adg adg commented Feb 24, 2016

master.Overlay("overlay").Parse("some other template")

What would that even do, conceptually? It makes no sense to me.

(my username is @adg not @agl 😄)

@bep
Copy link
Contributor Author

@bep bep commented Feb 24, 2016

What would that even do, conceptually? It makes no sense to me.

It would parse the overlay and associate it in the same namespace with the rest. Conceptually it would be the same as:

master.New("some other").Parse("some other template")

In the above, master.Lookpup("some other") does return the expected template.

Maybe my construct would make more sense looking like this:

master.New("some overlay").Overlay("some other template")

Anyhow, I would expect the master and the overlay to be associated (I believe the term associated is used in the template code).

(sorry about the username confusion, but agl is about as close to your real name as adg ..)

@faxal
Copy link

@faxal faxal commented Feb 24, 2016

I maybe missing something significant but why not keep overlay information in templates themselves?.

layout.tmpl

<html>
<body>
    {{block "content"}}
        Default content
    {{end}}
</body>
</html>

home.tmpl

{{template "layout.tmpl"}}
{{define "content"}}
    <div id="home">
    this overrides default content
    </div>
{{end}}

Something like this http://play.golang.org/p/qRmZPnKYCF

@adg
Copy link
Contributor

@adg adg commented Feb 24, 2016

@fazal you'll need to describe your proposal in a lot more detail. There
are many unanswered questions there.

On 24 February 2016 at 21:39, fazal notifications@github.com wrote:

I maybe missing something significant but why not keep overlay information
in templates themselves?.

layout.tmpl

{{block "content"}} Default content {{end}}

home.tmpl

{{template "layout.tmpl"}}
{{define "content"}}


this overrides default content

{{end}}


Reply to this email directly or view it on GitHub
#14285 (comment).

@rsc rsc modified the milestones: Go1.8, Go1.7 May 18, 2016
@rsc
Copy link
Contributor

@rsc rsc commented Oct 10, 2016

As Andrew said, I don't see a clear proposal here. Closing this issue.

@rsc rsc closed this Oct 10, 2016
hoogway added a commit to hoogway/golang-project that referenced this issue Aug 22, 2017
Fixes golang/go#14285

Change-Id: I3b32b7fc19960a77778e84cba2a0a95b49dbdf16
Reviewed-on: https://go-review.googlesource.com/19443
Reviewed-by: Rob Pike <r@golang.org>
Reviewed-by: Chris Broadfoot <cbro@golang.org>
Reviewed-by: Russ Cox <rsc@golang.org>
@golang golang locked and limited conversation to collaborators Oct 10, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
9 participants
You can’t perform that action at this time.