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

html/template: asymmetry of text/template and html/template definition #23990

Closed
ikrabbe opened this issue Feb 21, 2018 · 7 comments
Closed

html/template: asymmetry of text/template and html/template definition #23990

ikrabbe opened this issue Feb 21, 2018 · 7 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@ikrabbe
Copy link
Contributor

ikrabbe commented Feb 21, 2018

My current go version is go version go1.9.3 linux/amd64 but this architectural design flaw is not affected by any version. It's just part of the main line.
Also this problem is part of the definition of text/template and html/template definitions, and is not affected by the builder or target operating system.

I tried to unify text/template and html/template handling

For a recipe on how to see the problem, please see the Go Forum discussion.

Basically it is the documented difference of the Templates() call of either package that behave different from documentation to production.

text: Templates returns a slice of defined templates associated with t.
html: Templates returns a slice of the templates associated with t, including t itself.

We should redefine html/template to not include t itself, as its far more simple to add the root template t itself, instead of finding the root template t in the returned slice and remove that.

For the full details and more in-depth discussion, please read the forum posts.

@ALTree ALTree changed the title Asymmetry of text/template and html/template definition html/template: asymmetry of text/template and html/template definition Feb 21, 2018
@ALTree ALTree added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Feb 21, 2018
@ALTree
Copy link
Member

ALTree commented Feb 21, 2018

Pasting here rsc's comment on the Go forum, for completeness:

I think this is a bug. Either cmds should be returned in the first, or html should not be returned in the second. Please file an issue at golang.org/issue/new. Thanks.

Labelled this as needsDecision since it needs to be decided which package to modify.

@ALTree ALTree added this to the Go1.11 milestone Feb 21, 2018
@rsc
Copy link
Contributor

rsc commented Mar 27, 2018

https://play.golang.org/p/DCTbO6SEruf shows that the actual problem is text's .Templates dropping all templates with no Parsed definitions. It should probably list all, like html. In any event the doc distinction is wrong.

@rsc rsc added the NeedsFix The path to resolution is known, but the work has not been done. label Mar 27, 2018
@gopherbot gopherbot removed the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Mar 27, 2018
@ikrabbe
Copy link
Contributor Author

ikrabbe commented Mar 27, 2018

Though I think, that undefined (not parsed) templates are still member of the association, they should be returned by text/template, not including itself can still be quite useful to simply distinguish between root and associated templates, as if you associate a template with the same name as the root template, it will overwrite the root template name in the set, in case of the html/template.

Also comparing the internal definitions I would prefer to embed text/template.Template in html/template.Template as I'm still not convinced about this comment, from html/template/template.go:22,24

	// We could embed the text/template field, but it's safer not to because
	// we need to keep our version of the name space and the underlying
	// template's in sync.

and fix the name compilation in (text/template.Template).Templates(). But I fear I cannot provide a working fix until close of 1.11

And even more: https://play.golang.org/p/ye0hz7Yqfrr or this one https://play.golang.org/p/MIFqkA1DnU4
the order of definition affects the association.

@robpike
Copy link
Contributor

robpike commented May 14, 2018

The definition of t.Templates is "Templates returns a slice of defined templates associated with t." And the definition of "defined" is "has a parsed tree associated with it". So the text/templates package is working as documented, and as I understand it should behave. I believe that html/template is the one violating the contract here.

@robpike
Copy link
Contributor

robpike commented May 14, 2018

If you believe text/template should behave differently, that's a separate argument.

@robpike
Copy link
Contributor

robpike commented May 14, 2018

Notice that the documentation in the html package is different:

// Templates returns a slice of the templates associated with t, including t
// itself.

I'm inclined to leave this all alone. The code is subtle due to the design decision to not have template sets, which I think was a mistake.

@ikrabbe
Copy link
Contributor Author

ikrabbe commented Jun 16, 2018

I really thought about this for a while now and I agree with your observation, that it is really hard to change the template code. On the other hand, I would like to have a smarter and more organic solution for the templates, which seems to be some work, though.
As I currently plan on writing a service, that is controlled by some sets of template data, combining text and html, I might get annoyed enough, one day, to do so.
Until then I will close this issue: The package works as documented, though the two libraries behave different, where they should do the same. The usage of templates seems to be a very important pattern, for several use cases.

@ikrabbe ikrabbe closed this as completed Jun 16, 2018
@golang golang locked and limited conversation to collaborators Jun 16, 2019
@rsc rsc unassigned robpike Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

5 participants