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

proposal: text/template: Move text/template/parse into an internal package #25357

Closed
earthboundkid opened this issue May 11, 2018 · 16 comments
Closed
Labels
FrozenDueToAge Proposal Proposal-FinalCommentPeriod v2 A language change or incompatible library change
Milestone

Comments

@earthboundkid
Copy link
Contributor

For #10608, the text/template/parse package was changed in a way that was a breaking change in 28c1ad9. The justification of this is that text/template/parse would have been an internal package if that had existed before and users shouldn't import it. I agree with that reasoning, but I think if we are going to treat text/template/parse as internal and break its API, we may as well make it internal.

My proposal is that in Go 1.11, we add documentation explaining that text/template/parse will be moved to an internal package and in 1.12, text/template/parse is moved. If we want to be more aggressive, we could move it sooner, since 1.11 is already a breaking change to anyone relying on text/template/parse.

@cznic
Copy link
Contributor

cznic commented May 11, 2018

I don't think #10608 was a breaking change, but this issue definitely would be one.

@kardianos
Copy link
Contributor

@carlmjohnson I use the text/template parse tree. So... aside from breaking the Go1 compatibility promise which would prevent this, this is a really bad idea.

@earthboundkid
Copy link
Contributor Author

earthboundkid commented May 11, 2018

From the CL comments:

Daniel Martí:

Sorry, I'm still not clear on whether we can break the text/template/parse API. I would generally assume not, but the package doc clearly states that it's "shared internal data structures not intended for general use", and you gave your +2, so perhaps it is.

Rob Pike:

As long as html/template still works, yes.

This will break some dependants, no doubt, but parse would be an internal package if internal packages existed when it was written. The documentation says to stay away.

I would still be happy to abandon this, but the proposal was accepted so you are free to submit.

Brad Fitz:

LGTM I guess. I didn't know we had such a volatile, internal-but-public package.

@earthboundkid
Copy link
Contributor Author

earthboundkid commented May 11, 2018

If we want parse to follow the Go 1 guarantee, someone can go back and fix it, but as is, the guarantee is broken by token renaming.

@earthboundkid
Copy link
Contributor Author

I think if someone introduces type VariableNode = AssignNode that would fix the breakage.

@bradfitz bradfitz changed the title text/template: Move text/template/parse into an internal package proposal: text/template: Move text/template/parse into an internal package May 11, 2018
@gopherbot gopherbot added this to the Proposal milestone May 11, 2018
@robpike
Copy link
Contributor

robpike commented May 11, 2018

The doc comment suggests not to use this package at all. It was created before internal packages were available but if they were, it would have been made one.

So it's in an odd state now. It has had several breaking changes, and there may be more, because it's really not a package you should be using. But people do use it, so we can't just make it internal.

@robpike robpike closed this as completed May 11, 2018
@robpike robpike reopened this May 11, 2018
@mvdan
Copy link
Member

mvdan commented May 12, 2018

Why not simply mark this issue as Go2? It seems to me like that is the right time to start hiding this package, if that's what we want.

@robpike robpike added the v2 A language change or incompatible library change label May 12, 2018
@robpike
Copy link
Contributor

robpike commented May 12, 2018

It's not at all clear we can do this in Go 2 either, but we certainly can't do it in Go 1.

@Azareal
Copy link

Azareal commented May 13, 2018

I use it for a Go Template to Go transpiler using the same AST, I would use the other packages, however I've found that they can be fairly... slow in comparison. It's as much as 40 times faster.

@earthboundkid
Copy link
Contributor Author

#25875 also ran into issues with changes to text/template/parse.

What about making a new internal package and using that in text/template and just leaving the 1.10 version of text/template/parse around as a fossil? That seems less likely to break people, if that's the worry.

@earthboundkid
Copy link
Contributor Author

Look like the same suggestion was made at #25968 (comment).

@ianlancetaylor
Copy link
Contributor

Perhaps a template parsing package should be available as a third party package. It doesn't seem that we want to make that package be part of the (non-internal) standard library, since we've had to change the API in the past and may have to change it in the future.

@ianlancetaylor ianlancetaylor added the Go2Cleanup Used by Ian and Robert for Go 2 organization. Unless you’re Ian or Robert, please do not use this. label Sep 12, 2018
@draeron
Copy link
Contributor

draeron commented Sep 27, 2018

Actually i would go the opposite way. I have use case for requiring to be able to list a template's complete parse tree because i want to list all fields that are accessed in it.

But the package does obfuscation through the Node interface preventing casting to proper node type (ActionNode etc.) Unless there is some obscure way to to this which i haven't figure out.

I understand why someone might want to create true black box implementation but i don't how this would serve any useful purpose.

my 2 cents.

@rsc
Copy link
Contributor

rsc commented Apr 22, 2020

Based on the discussion over on #34652, this sounds like it should be rejected.

@ianlancetaylor ianlancetaylor removed the Go2Cleanup Used by Ian and Robert for Go 2 organization. Unless you’re Ian or Robert, please do not use this. label Apr 22, 2020
@ianlancetaylor
Copy link
Contributor

Based on the discussion above, this is a likely decline.

Leaving open for four weeks for final comments.

@ianlancetaylor
Copy link
Contributor

No further comments. Closing.

@golang golang locked and limited conversation to collaborators Jun 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge Proposal Proposal-FinalCommentPeriod v2 A language change or incompatible library change
Projects
None yet
Development

No branches or pull requests

10 participants