-
Notifications
You must be signed in to change notification settings - Fork 558
JSON serialization with optional HTML escaping #1876
Conversation
@anhowe @JackQuincy would be great to get your feedback on this change. I'm not aware of any need to HTML-escape |
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.
looks good two small feedbacks.
package helpers | ||
|
||
import ( | ||
// "fmt" |
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.
?
) | ||
|
||
// JSONMarshalIndent marshals formatted JSON w/ optional SetEscapeHTML | ||
func JSONMarshalIndent(content interface{}, prefix, indent string, escape bool) ([]byte, 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.
escape only makes sense if you read the method comments. Which I guess that is okay. Other wise from the method name and variable name I'm not sure what is being escaped. Is that normal in go? I know go likes brevity, but I'm not sure if naming depending on the user reading the comment is normal. Also I don't think we ever set escape to true. So we could just make it assumed as false.
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!
We could change the name to JSONMarshalIndentWithEscape
, but kinda long.
The reason I made it configurable is because I wasn't sure if we'd never want to include the standard golang implementation (angle brackets "<" and ">" are escaped to "\u003c" and "\u003e")
What this PR does / why we need it: See the description of this method here:
https://golang.org/pkg/encoding/json/#Marshal
The important part:
We aren't sensitive to the HTML browser issue described above, and we are unknowingly doing this conversion in places where we don't want to be.
Release note: