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

Jump to bottom

text/template/parse: add CommentNode to the parse tree #34652

Closed
a8m opened this issue Oct 2, 2019 · 19 comments
Closed

text/template/parse: add CommentNode to the parse tree #34652

a8m opened this issue Oct 2, 2019 · 19 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Proposal Proposal-Accepted
Projects
Proposals (old)
Milestone
Backlog

Comments

@a8m
Copy link
Contributor

a8m commented Oct 2, 2019

Currently, template comments are ignored in the lexer and do not show in the parse tree.
It makes it difficult to build tools around templates (like code formatter) without re-implementing the parser.

My proposal is to add a CommentNode to the parse tree and skip it in the executer.
@andybons andybons changed the title text/template/parse: add CommentNode to the parse tree proposal: text/template/parse: add CommentNode to the parse tree Oct 2, 2019
@gopherbot gopherbot added this to the Proposal milestone Oct 2, 2019
@mvdan
Copy link
Member

mvdan commented Oct 4, 2019

I assume this can only be accepted if #25357 is rejected.

@a8m
Copy link
Contributor Author

a8m commented Oct 4, 2019

I assume this can only be accepted if #25357 is rejected.

Why? It's a Go2 proposal, and text/template/parse is a public package that's being used outside.

@mvdan
Copy link
Member

mvdan commented Oct 10, 2019

Yes; but if the plan is to eventually hide/remove the template/parse package, considering API changes to it wouldn't make a lot of sense. At the very least, the two proposals are related.

@rsc rsc added this to Incoming in Proposals (old) Dec 4, 2019
@rsc
Copy link
Contributor

rsc commented Apr 8, 2020

/cc @robpike for thoughts about supporting template-manipulation programs a la gofmt by adding CommentNode.

@rsc rsc moved this from Incoming to Active in Proposals (old) Apr 8, 2020
@a8m
Copy link
Contributor Author

a8m commented Apr 8, 2020 •
edited

I think it’s needed for gopls as well - #36911

@rsc rsc mentioned this issue Apr 8, 2020
Open
@robpike
Copy link
Contributor

robpike commented Apr 8, 2020

I would have made the package internal had that been an option at the time, but I couldn't, and now it's being used by software that wouldn't have been able to, making it harder to maintain and de facto subject to the compatibility rules. From my position, that is a tale of woe. Hyrum's law, which was a problem before Hyrum was even born, bites again.

On the other hand, people do depend on it and the uses are mostly reasonable, so we should probably accept this and close #25357. I do think though that we should keep the comment that discourages its use, to give us some leverage against unreasonable requests for change.

@rsc
Copy link
Contributor

rsc commented Apr 15, 2020

Based on the discussion above, then, this sounds like a likely accept.
For what it's worth, I might have made regexp/syntax an internal package too, but it has proved so useful in so many other contexts that I'm glad I exposed it.

@rsc rsc moved this from Active to Likely Accept in Proposals (old) Apr 15, 2020
@mvdan
Copy link
Member

mvdan commented Apr 15, 2020

Sounds good to me. Should we reject #25357, then? In the past few years it's been made clear that Go2 will be an incremental process instead of one big set of breaking changes, so it seems unlikely to me that we'd hide an entire package at any point.

@a8m
Copy link
Contributor Author

a8m commented Apr 15, 2020

Thanks for the feedback. I would be happy to add this once it's accepted.

Should we reject #25357, then?

IMHO, the template package should provide an API for tools like gopls to interact with it. Like the go package for Go.
Otherwise, what's the alternative, reimplementing it externally? This is what I do currently, but I expect to replace it once gopls will add support for it (#36911).

@rsc
Copy link
Contributor

rsc commented Apr 22, 2020

No change in consensus, so accepted. Yes, I think we can reject #25357 as well.
For better or worse, this package is the equivalent of go/ast for templates.
In a different timeline we might have made it an internal-only package and
prepared an API that was specifically meant for public consumption, but that's
not what happened.

@rsc rsc moved this from Likely Accept to Accepted in Proposals (old) Apr 22, 2020
@rsc rsc mentioned this issue Apr 22, 2020
Closed
@earthboundkid
Copy link
Contributor

If this has been accepted, then I take it there's no interest in marking text/template/parse as deprecated?

@a8m
Copy link
Contributor Author

a8m commented Apr 22, 2020 •
edited

If this has been accepted, then I take it

Since I already have a working change for this in my fork that I plan to push in the upcoming days, I prefer to take it. Thanks

@rsc rsc modified the milestones: Proposal, Backlog Apr 22, 2020
@rsc rsc added NeedsFix The path to resolution is known, but the work has not been done. Proposal-Accepted and removed Proposal-FinalCommentPeriod labels Apr 22, 2020
@rsc rsc changed the title proposal: text/template/parse: add CommentNode to the parse tree text/template/parse: add CommentNode to the parse tree Apr 22, 2020
@gopherbot
Copy link

Change https://golang.org/cl/229398 mentions this issue: text/template: add CommentNode to template parse tree

@a8m a8m mentioned this issue Apr 23, 2020
Closed
@ianlancetaylor
Copy link
Contributor

The current CL is adding a new ParseWithOptions function; see #38627.

@a8m
Copy link
Contributor Author

a8m commented May 6, 2020

Hey, CL229398 is currently blocked on review. There's another discussion on #38627 about making the API more generic for changes in the future.

Any chance to get help with the review? I really want to see it shipped with 1.15.

@gopherbot
Copy link

Change https://golang.org/cl/301493 mentions this issue: text/template/parse: add mode to skip func-check on parsing

@gopherbot
Copy link

Change https://golang.org/cl/301493 mentions this issue: text/template/parse: add a mode to skip func-check on parsing

gopherbot pushed a commit that referenced this issue Apr 19, 2021
@a8m @ianlancetaylor
text/template/parse: add a mode to skip func-check on parsing
5780ab4 
Following the discussion on #34652 and the proposal of #36911 (gopls),
this CL adds an option to skip the function declartion check on parsing,
in order to make it possible to parse arbitrary template text files and
get their AST.

Fixed #38627

Change-Id: Id1e0360fc726b49dcdd49716ce25563ebaae6c10
Reviewed-on: https://go-review.googlesource.com/c/go/+/301493
Reviewed-by: Rob Pike <r@golang.org>
Reviewed-by: Emmanuel Odeke <emmanuel@orijtech.com>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Trust: Ian Lance Taylor <iant@golang.org>
@gopherbot
Copy link

Change https://golang.org/cl/317269 mentions this issue: text/template/parse: rename DeferFuncCheck to SkipFuncCheck

gopherbot pushed a commit that referenced this issue May 6, 2021
@rsc
text/template/parse: rename DeferFuncCheck to SkipFuncCheck
0e7a7a6 
The proposal as accepted in #34652 named the bit SkipFuncCheck.
It was renamed to DeferFuncCheck during the code review on a suggestion by Rob,
along with a comment to “defer type checking functions until template is executed,”
but this description is not accurate: the package has never type-checked functions,
only verified their existence. And the effect of the bit in this package is to eliminate
this check entirely, not to defer it to some later time.

I was writing code using this new bit and was very confused about when the
"type checking" was being deferred to and how to stop that entirely,
since in my use case I wanted no checks at all. What I wanted is what the bit does,
it just wasn't named accurately.

Rename back to SkipFuncCheck.

Change-Id: I8e62099c8a904ed04521eb5b86155290f6d5b12f
Reviewed-on: https://go-review.googlesource.com/c/go/+/317269
Trust: Russ Cox <rsc@golang.org>
Trust: Daniel Martí <mvdan@mvdan.cc>
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Rob Pike <r@golang.org>
@gopherbot
Copy link

Change https://golang.org/cl/311569 mentions this issue: doc/go1.17: document text/template.DeferFuncCheck

gopherbot pushed a commit that referenced this issue May 29, 2021
@a8m @ianlancetaylor
doc/go1.17: document text/template/parse.SkipFuncCheck
f6cc392 
Documents the newly added mode that skips type checking
functions as per CL 301493.

Fixes #46025
For #34652
For #44513
For #38627

Change-Id: I56c4f65924702a931944796e39f43cfeb66abc8a
Reviewed-on: https://go-review.googlesource.com/c/go/+/311569
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Emmanuel Odeke <emmanuel@orijtech.com>
Trust: Michael Knyszek <mknyszek@google.com>
@golang golang locked and limited conversation to collaborators May 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Proposal Proposal-Accepted
Projects
No open projects
Proposals (old)
Accepted
Milestone
Backlog
Development

No branches or pull requests
7 participants
@rsc @earthboundkid @ianlancetaylor @mvdan @robpike @a8m @gopherbot