Skip to content

JSON JS escaping#1736

Merged
farmdawgnation merged 3 commits into
masterfrom
json-js-escaping
Dec 2, 2015
Merged

JSON JS escaping#1736
farmdawgnation merged 3 commits into
masterfrom
json-js-escaping

Conversation

@Shadowfiend
Copy link
Copy Markdown
Member

I added a few scaladocs beyond the core work. The main commit:

Until now, we've had a hardcoded set of characters that would always be escaped
in JSON; these were never modifiable. We now plug in to the new RenderSettings
case class to add the ability to specify sets of characters that should be
escaped.

Additionally, we add a default set of such characters that corresponds to the
characters that must be escaped when JSON is evaluated directly by a JavaScript
engine rather than by a valid JSON parser (as when using JSON-P).

Lastly, we provide two default RenderSettings instances, prettyJs and
compactJs, that encapsulate the appropriate escaping with pretty or compact
rendering.

Closes #1676.

Until now, we've had a hardcoded set of characters that would always be escaped
in JSON; these were never modifiable. We now plug in to the new RenderSettings
case class to add the ability to specify sets of characters that should be
escaped.

Additionally, we add a default set of such characters that corresponds to the
characters that must be escaped when JSON is evaluated directly by a JavaScript
engine rather than by a valid JSON parser (as when using JSON-P).

Lastly, we provide two default RenderSettings instances, prettyJs and
compactJs, that encapsulate the appropriate escaping with pretty or compact
rendering.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Maybe this should mention it's only for JSON instead of expecting people to click through to the render settings docs? Also, could we make the reference to those settings a link?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Not sure what you mean regarding “only for JSON”—it's in JsonAST and lift-json, right?

I'll definitely make them a link, good catch.

@farmdawgnation
Copy link
Copy Markdown
Member

I like it.

@Shadowfiend
Copy link
Copy Markdown
Member Author

We probably also need to change Lift's built-in stuff to use the right render settings :/

@Shadowfiend
Copy link
Copy Markdown
Member Author

Actually just kidding, this is only for stuff that parses JSON as JS, and the Lift JSON stuff generally is parsed as JSON on the browser, so we're good.

@Shadowfiend Shadowfiend modified the milestone: 3.0-M7 Dec 1, 2015
@Shadowfiend
Copy link
Copy Markdown
Member Author

@farmdawgnation dropped some links on the scaladocs 🎉

@farmdawgnation
Copy link
Copy Markdown
Member

👍

farmdawgnation added a commit that referenced this pull request Dec 2, 2015
Until now, we've had a hardcoded set of characters that would always be escaped
in JSON; these were never modifiable. We now plug in to the new RenderSettings
case class to add the ability to specify sets of characters that should be
escaped.

Additionally, we add a default set of such characters that corresponds to the
characters that must be escaped when JSON is evaluated directly by a JavaScript
engine rather than by a valid JSON parser (as when using JSON-P).

Lastly, we provide two default RenderSettings instances, prettyJs and
compactJs, that encapsulate the appropriate escaping with pretty or compact
rendering.
@farmdawgnation farmdawgnation merged commit 24cbbde into master Dec 2, 2015
@farmdawgnation farmdawgnation deleted the json-js-escaping branch December 2, 2015 03:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants