-
-
Notifications
You must be signed in to change notification settings - Fork 963
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
feat: courier templates fs support #2164
Conversation
courier/template/load_template.go
Outdated
var b bytes.Buffer | ||
if err := t.Execute(&b, model); err != nil { | ||
return "", err | ||
} | ||
return b.String(), nil | ||
} | ||
|
||
func LoadHTMLTemplate(osdir, name, pattern string, model interface{}) (string, error) { | ||
t, err := loadTemplate(osdir, name, pattern, true) | ||
func LoadHTMLTemplate(osdir, name, pattern string, model interface{}, fs ...embed.FS) (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.
It looks like we will only use the first embedFS argument. So maybe just make this a regular arg?
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.
but then it won't be optional anymore. we will then require the current functions to pass a nil i guess
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.
Three options:
- nil is acceptable (okayish)
- just pass the FS in the caller (preferable)
- if more options are anticipated add an options struct (least preferable because extra work that can easily be avoided, not many options planned in the future)
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 see, okay, I will implement option 2.
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.
Thank you @Benehiko :)
https://github.com/ory/kratos/blob/master/courier/template/load_template.go#L80-L87 I see it was added through 54b97b4 Okay got it now. Will keep it intact. |
Codecov Report
@@ Coverage Diff @@
## master #2164 +/- ##
==========================================
- Coverage 75.24% 75.22% -0.03%
==========================================
Files 294 294
Lines 15618 15618
==========================================
- Hits 11752 11748 -4
- Misses 3032 3035 +3
- Partials 834 835 +1
Continue to review full report at Codecov.
|
Great stuff, thank you @Benehiko ! |
Adds
embed.FS
support to the templates.Related issue(s)
Checklist
introduces a new feature.
contributing code guidelines.
vulnerability. If this pull request addresses a security. vulnerability, I
confirm that I got green light (please contact
[email protected]) from the maintainers to push
the changes.
works.
Further Comments