Only escape compulsory characters for JSON by default (#2137) #2138
Conversation
Burgch
force-pushed
the
json-unicode-escape
branch
from
52ffdce
to
170880a
Compare
|
Fixed a bad merge in |
Foundation/src/JSONString.cpp
Outdated
| { | ||
| if (wrap) out << '"'; | ||
| out << UTF8::escape(value.begin(), value.end()); | ||
| if (escapeAllUnicode) { |
| @@ -1933,7 +1933,7 @@ void JSONTest::testEscapeUnicode() | |||
| result = parser.parse(utf8Text); | |||
| object = result.extract<Object::Ptr>(); | |||
| ss.str(""); object->stringify(ss); | |||
| assert (ss.str() == "{\"name\":\"g\\u00FCnter\"}"); | |||
| assert (ss.str() == "{\"name\":\"g\xC3\xBCnter\"}"); | |||
There was a problem hiding this comment.
I'd leave this test as is and create another one that tests functionality without escaping. This change will break the code, so there should be a way to force the parser to escape.
There was a problem hiding this comment.
I disagree - the code was broken by 1.8.0, and this is returning it back to the former functionality. This test would fail as is with versions < 1.8.0
There was a problem hiding this comment.
Perhaps I did not state it correctly. This test is named testEscapeUnicode and it should do what its name implies - test (un)escaping the unicode. What was before does not matter, we should do the right thing now. And that includes allowing end user to choose whether the returned JSON string is unicode-escaped or not - probably just change in the JSON::stringifier.
There was a problem hiding this comment.
I'm not able to commit much more time on this; is there a small tweak to the PR that would mean you'd accept it? Perhaps renaming the test to testParsingUnicode, which is most of what it does anyway?
There was a problem hiding this comment.
That would be fine, but the main concern is that a json containing unicode characters can not be escaped when stringified (that does not look like a big change, either).
There was a problem hiding this comment.
They don't need to be escaped though - any compliant JSON parser must accept the non-escaped characters as well as the escaped ones, as this test is demonstrating.
There was a problem hiding this comment.
user puts in
"{ \"name\" : \"\\u4e2d\" }"
does something with it (add or remove something), and gets out
"{\"name\":\"\xE4\xB8\xAD\"}"
now, it is true that those things are equivalent for JSON and a compliant parser must accept them both. but from my experience, people can be more picky than parsers, especially about the superficial things. I'll finish it, thanks for reporting the issue and the fixes so far.
There was a problem hiding this comment.
Thanks very much, appreciate it!
|
@Burgch Somehow I ended up commenting in your repo, so both those comments and the ones in pull are showing here. In any case - overall, I agree with the pull and reasoning that escaping should not be default, but since we are breaking existing functionality here, somehow JSON library should have the option to escape, too (didn't yet look into details of what would be the best way to achieve this). |
Burgch
force-pushed
the
json-unicode-escape
branch
from
170880a
to
38d5b53
Compare
|
moved to #2145 |
Fixes #2137