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/parse: add an option to skip functions existence check on parsing #38627

Open
a8m opened this issue Apr 23, 2020 · 4 comments
Labels
Milestone

Comments

@a8m
Copy link
Contributor

@a8m a8m commented Apr 23, 2020

Following the discussion on #34652 and the proposal of #36911, I'm proposing to add an option to skip the function existence check on parsing, in order to make it possible to parse an arbitrary template text and get its AST.

Currently, if we parse the following template text {{ title | "foo" }} - without passing the functions map we'll get a "function "title" not defined" error. This is not ideal if we plan to add tooling support around the template package (like gopls).

In CL229398, I'm suggesting to add support for parsing template comments, and in order to keep backward compatibility the API looks as follows:

func ParseWithOptions(name, text string, options ...Option) (map[string]*Tree, error)

If we'll go with this version, I'm proposing to add another option (like SkipFuncCheck) to the parse options.

@gopherbot gopherbot added this to the Proposal milestone Apr 23, 2020
@gopherbot gopherbot added the Proposal label Apr 23, 2020
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Apr 23, 2020

I don't see a need for the Option type here. Can we just add some flags?

@a8m
Copy link
Contributor Author

@a8m a8m commented Apr 24, 2020

I don't see a need for the Option type here. Can we just add some flags?

Thanks for the response @ianlancetaylor. Can you give an example how the API will look like with flags? I'm not sure how we can add multiple flags to the current Parse function:

Parse(name, text, leftDelim, rightDelim string, funcs ...map[string]interface{}) (map[string]*Tree, error)

The idea is to allow parsing template text with comments and ignoring the functions existence check. This is quite necessary if we want to support gopls or want to make is possible to develop tools like gofmt for templates.

The API with options will look as follows:

ParseWithOptions(name, text, WithComments(), WithFuncs(funcs...), ...)

SkipFuncCheck is another option. LeftDelim and RightDelim can be added to options as well. It'll make the API much more flexible for changes in the future.

I've suggested ParseWithOptions because it's used by net/html.ParseWithOptions, but a Config object can work as well.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Apr 25, 2020

I suppose it makes sense to wrap in LeftDelim and RightDelim as options. I don't see a reason to put funcs in options, but in a config struct it would make sense. Let's use a config struct.

Sorry, I'm not sure what you mean by net/html.ParseWithOptions, there is no such function and I'm not sure what you are referring to.

@a8m
Copy link
Contributor Author

@a8m a8m commented Apr 25, 2020

I don't see a reason to put funcs in options, but in a config struct it would make sense. Let's use a config struct.

Sure. Let's use a config struct.

Sorry, I'm not sure what you mean by net/html.ParseWithOptions, there is no such function and I'm not sure what you are referring to.

ParseWithOptions is a function under the golang.org/x/net/html package. See: https://godoc.org/golang.org/x/net/html#ParseWithOptions

The golang.org/x/text repository contains a few packages that use functional options as well, so I just wasn't sure what to use, a config struct or functional options.

Thanks for the feedback and I'll continue the discussion on Gerrit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.