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

Add an optional boolean to not escape html during json encoding #891

Merged
merged 1 commit into from
Mar 13, 2024

Conversation

WnP
Copy link
Contributor

@WnP WnP commented Jan 29, 2024

Fix #890

@prembhaskal
Copy link
Contributor

checking.
one comment about boolean flag, can we also explore if we can pass additional parameters to the json.encode() method. (just a thought I am not sure if it is feasible to do).

@Okhoshi
Copy link

Okhoshi commented Jan 30, 2024

one comment about boolean flag, can we also explore if we can pass additional parameters to the json.encode() method. (just a thought I am not sure if it is feasible to do).

There's no other option on the json Encoder, only SetIndent and SetEscapeHTML.

@WnP
Copy link
Contributor Author

WnP commented Jan 30, 2024

I've fixed the tests and created one for the new feature.

@WnP WnP force-pushed the json-no-escape-html branch 2 times, most recently from 4bf88c3 to b5de38f Compare January 30, 2024 10:09
@prembhaskal
Copy link
Contributor

prembhaskal commented Jan 30, 2024

one comment about boolean flag, can we also explore if we can pass additional parameters to the json.encode() method. (just a thought I am not sure if it is feasible to do).

There's no other option on the json Encoder, only SetIndent and SetEscapeHTML.

ignore my previous comment, I think I had a brain fade moment there.

I feel it is better to name the variable as escape_html with the default value as true, which will be inline with what json encoder is doing.

someone intending to not escape html can set it as escape_html=false

pkg/yttlibrary/json.go Outdated Show resolved Hide resolved
@prembhaskal
Copy link
Contributor

@cppforlife @100mik please have a look at this when you get time.

@prembhaskal
Copy link
Contributor

@WnP also could you please check the lint failure when you get time.
meanwhile I will try to get our approvers eyes on the review.

Signed-off-by: Steeve Chailloux <me@steeve.io>
@WnP
Copy link
Contributor Author

WnP commented Feb 8, 2024

Sorry for the delay @prembhaskal, I was quite busy during the last days.

Feel free to ping me if there's something missing.

@prembhaskal
Copy link
Contributor

LGTM,
@cppforlife please review when you get time.

@sethiyash
Copy link
Contributor

LGTM!

Copy link
Member

@joaopapereira joaopapereira left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@joaopapereira joaopapereira merged commit 6cad190 into carvel-dev:develop Mar 13, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Add an optional boolean to not escape html during json encoding
5 participants