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/parse: backwards incompatible changes #25968

Closed
dsnet opened this issue Jun 19, 2018 · 15 comments
Closed

text/template/parse: backwards incompatible changes #25968

dsnet opened this issue Jun 19, 2018 · 15 comments

Comments

@dsnet
Copy link
Member

@dsnet dsnet commented Jun 19, 2018

https://golang.org/cl/84480 deliberately makes some breaking API changes saying:

Modifying the text/template/parse package in a backwards incompatible
manner is acceptable, given that the package godoc clearly states that
it isn't intended for general use. It's the equivalent of an internal
package, back when internal packages didn't exist yet.

Additionally, the documentation in text/template/parse says:

Clients should use those packages to construct templates rather than this one, which provides shared internal data structures not intended for general use.

Saying that the package is "not intended for general use" is not the same thing as "this package must not be used by any package other than text/template". As a result, there exist code that depends on the parse package that are now broken by this change and I don't believe it is okay to break them.

According to the Go 1 compatibility document:

Go 1 defines two things: first, the specification of the language; and second, the specification of a set of core APIs, the "standard packages" of the Go library.

It isn't defined what the set of "standard packages" are, but unless an exception is provided, I think it is reasonable to expect that parse is considered one of them (even with the disclaimer and even if this would have been an internal-only package had they existed when the standard library was first created).

\cc @mvdan @robpike @cybrcodr @bradfitz

@dsnet dsnet added this to the Go1.11 milestone Jun 19, 2018
@dsnet dsnet added the NeedsDecision label Jun 19, 2018
@bontibon

This comment has been minimized.

Copy link
Contributor

@bontibon bontibon commented Jun 19, 2018

Some related discussion is over here: #25357

@dsnet

This comment has been minimized.

Copy link
Member Author

@dsnet dsnet commented Jun 19, 2018

For some background, the last time parse was changed in an incompatible was for Go1.1. See https://golang.org/cl/6586074 and https://golang.org/cl/6576058. Thus, the API has been mostly stable for the past 6 years.

@robpike

This comment has been minimized.

Copy link
Contributor

@robpike robpike commented Jun 20, 2018

I remain happy not to have this change at all, but seem to be in the minority. People want the feature.

@opennota

This comment has been minimized.

Copy link

@opennota opennota commented Jun 20, 2018

@dsnet What action do you propose? Drop the change? Re-rename the renamed public types/variables/etc back (is that enough?)?

@mvdan

This comment has been minimized.

Copy link
Member

@mvdan mvdan commented Jun 20, 2018

I would have originally thought that breaking the parse package was not possible, but since Rob approved the change and gave a reason in favor of doing so, I didn't give it more thought.

I can have a look at undoing part of the changes to the parse package, so that Go1 compatibility is kept without reverting the entire CL and feature. That could be a short-term solution, while #25357 is more of a long-term one.

As for the text/template feature - I imagine that it's not the point of this issue to undo the entire CL. There was demand for it and the proposal was accepted. But if someone wants to make a point to also revert the feature before 1.11 is out, now is the time to speak up :)

@dsnet

This comment has been minimized.

Copy link
Member Author

@dsnet dsnet commented Jun 20, 2018

This is breaking a handful of targets inside Google. Personally, I don't think people should be depending on parse directly either. I believe we should either strongly document that parse is not supported by the compatibility guarantee or we should actually follow Go1 compatibility. Anything in between seems like madness. I don't have an opinion as to either position we take.

@mvdan

This comment has been minimized.

Copy link
Member

@mvdan mvdan commented Jun 20, 2018

@dsnet I agree, and I too find either option acceptable. @robpike @rsc @rogpeppe any thoughts?

@robpike

This comment has been minimized.

Copy link
Contributor

@robpike robpike commented Jun 21, 2018

I don't think you can tell people not to depend on the package - many already do. The real question is whether it's OK to break those that do. We've vacillated on that point, but we have done it in the past.

So: Can we document that we reserve the right to break users of this package?

@josharian

This comment has been minimized.

Copy link
Contributor

@josharian josharian commented Jun 21, 2018

In an attempt to have our cake and eat it too, we could revert text/template/parse to its 1.10 state, freeze it, duplicate it to text/template/internal/parse, update text/template to use the internal package, and then re-apply the 1.11 changes there. The result is duplicated code, but since the non-internal package is frozen, that shouldn’t hurt too much.

@jimmyfrasche

This comment has been minimized.

Copy link
Member

@jimmyfrasche jimmyfrasche commented Jun 21, 2018

{text,html}/template use types from parse

@josharian

This comment has been minimized.

Copy link
Contributor

@josharian josharian commented Jun 21, 2018

internal/template/parse, then. Ugly, true.

@jimmyfrasche

This comment has been minimized.

Copy link
Member

@jimmyfrasche jimmyfrasche commented Jun 21, 2018

sorry, use types from parse in their public apis. Those wouldn't work right unless the internal/parse library types are aliases and at that point not much has been gained.

@kardianos

This comment has been minimized.

Copy link
Contributor

@kardianos kardianos commented Jun 21, 2018

May I present option (3): we introduce a CL that both keeps the new functionality and still retains most, if not all compatibility. Unless I'm missing something, this would involve:

  • Rename AssignNode back to VariableNode (or creating an alias for that name).
  • Rename PipeNode.Decl to PipeNode.IsDecl.
  • Rename PipeNode.Vars back to PipeNode.Decl.

I love the new template feature (I've wanted it on many occasions in the past) and I use the template/parse package directly too. Let's do both. I've read through the associated CL a few times, I'm not seeing a good reason why the API was broken in the first place.

@mvdan

This comment has been minimized.

Copy link
Member

@mvdan mvdan commented Jun 21, 2018

@kardianos yes, that's what I was suggesting in my first comment, just haven't found the time to write the CL yet :)

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Jun 21, 2018

Change https://golang.org/cl/120355 mentions this issue: text/template/parse: do not break API

@gopherbot gopherbot closed this in bedfa4e Jun 22, 2018
anthonyfok added a commit to gohugoio/hugo that referenced this issue Jun 23, 2018
Go developers have undone the breaking API changes
in the following commit:

commit bedfa4e1c37bd08063865da628f242d27ca06ec4
Author: Daniel Theophanes <kardianos@gmail.com>
Date:   Thu Jun 21 10:41:26 2018 -0700

    text/template/parse: undo breaking API changes

    golang.org/cl/84480 altered the API for the parse package for
    clarity and consistency. However, the changes also broke the
    API for consumers of the package. This CL reverts the API
    to the previous spelling, adding only a single new exported
    symbol.

    Fixes #25968

    Change-Id: Ieb81054b61eeac7df3bc3864ef446df43c26b80f
    Reviewed-on: https://go-review.googlesource.com/120355
    Reviewed-by: Daniel Martí <mvdan@mvdan.cc>
    Reviewed-by: Rob Pike <r@golang.org>
    Run-TryBot: Daniel Martí <mvdan@mvdan.cc>
    TryBot-Result: Gobot Gobot <gobot@golang.org>

See golang/go#25968

This reverts commit 9f27091.

Closes #4784
Fixes #4873
@golang golang locked and limited conversation to collaborators Jun 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
9 participants
You can’t perform that action at this time.