-
Notifications
You must be signed in to change notification settings - Fork 48
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
Allow defining template user functions #74
Conversation
pkg/template/template.go
Outdated
} | ||
} | ||
|
||
// RegisterFunc registers a named function or overwrites a built-in function. | ||
// Call this before Parse. | ||
func (t *Template) RegisterFunc(name string, f func(fields ...interface{}) (string, error)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only question I have is should we change this to be Funcs
and have the argument be a template.FuncMap
to match the text/template
package? My vote would be yes to keep consistency with the text/template
package since we have been doing that with Parse
and Execute
. I know I was the one who suggested RegisterFunc
in the first place 😞
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been trying to think of something along these lines as well. Having written an example for testing, the user ends up having to do a lot of what text/template
does internally.
Do we support devs calling RegisterFuncs
multiple times and just merge the maps? Last wins, which would be consistent with possibly overriding defaults?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is what I was thinking. We support calling Funcs
multiple times and just merging the maps where the latest entry for a key wins. That is how Funcs
works in text/template
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@heaths I went ahead and made the changes I suggested. Let me know what you think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making the changes. It was on my TODO list but a bit down the priority list.
Resolves #73