-
Notifications
You must be signed in to change notification settings - Fork 130
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
[MI-54922] Replaced the "text/template" package with "html/template" #988
Conversation
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 don't think this will work since there's a mixture of HTML and non-HTML templates that we use. Only the ones that are actually HTML should use html/template
.
@jupenur I have tested it for "application/json" and "markdown" templates, and everything seems to work fine. Also, in the documentation of html/template, it's mentioned that this package wraps "text/template". Please let me know your opinions on this.
|
@raghavaggarwal2308 In general, we should only use the |
@mickmister @jupenur Updated logic to use 'html/template' for HTML templates only. |
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.
LGTM
Co-authored-by: Abhishek Verma <[email protected]>
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #988 +/- ##
==========================================
+ Coverage 29.65% 29.69% +0.04%
==========================================
Files 52 52
Lines 7794 7812 +18
==========================================
+ Hits 2311 2320 +9
- Misses 5287 5295 +8
- Partials 196 197 +1
☔ View full report in Codecov by Sentry. |
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.
Tested and Approved
Working fine for both cloud and server, LGTM
Summary
Replaced the "text/template" package with "html/template"