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

Open
carlmjohnson opened this issue May 11, 2018 · 13 comments

Comments

@carlmjohnson
Copy link
Contributor

@carlmjohnson carlmjohnson commented May 11, 2018

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

This comment has been minimized.

Copy link
Contributor

@cznic cznic commented May 11, 2018

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

@kardianos

This comment has been minimized.

Copy link
Contributor

@kardianos kardianos commented May 11, 2018

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

@carlmjohnson

This comment has been minimized.

Copy link
Contributor Author

@carlmjohnson carlmjohnson 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.

@carlmjohnson

This comment has been minimized.

Copy link
Contributor Author

@carlmjohnson carlmjohnson 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.

@carlmjohnson

This comment has been minimized.

Copy link
Contributor Author

@carlmjohnson carlmjohnson commented May 11, 2018

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
@gopherbot gopherbot added the Proposal label May 11, 2018
@robpike

This comment has been minimized.

Copy link
Contributor

@robpike 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 May 11, 2018
@robpike robpike reopened this May 11, 2018
@mvdan

This comment has been minimized.

Copy link
Member

@mvdan 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 Go2 label May 12, 2018
@robpike

This comment has been minimized.

Copy link
Contributor

@robpike 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

This comment has been minimized.

Copy link

@Azareal 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.

@carlmjohnson

This comment has been minimized.

Copy link
Contributor Author

@carlmjohnson carlmjohnson commented Jul 10, 2018

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

@carlmjohnson

This comment has been minimized.

Copy link
Contributor Author

@carlmjohnson carlmjohnson commented Jul 10, 2018

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

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Sep 12, 2018

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.

@draeron

This comment has been minimized.

Copy link
Contributor

@draeron 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.