-
-
Notifications
You must be signed in to change notification settings - Fork 21.1k
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
JSON parser fix #40804
JSON parser fix #40804
Conversation
Nice, ideally this is where #40795 could've been merged to |
This fixes the first assert in #40795 (detected as parse error now)! But this assert fails:
Do you think it should return the string or return empty |
I'm not really sure, how this should be handled, but I would expect an empty or |
@Xrayez I've added results reset for malformed json, test case should not be failing now |
// Reset return value to empty `Variant` | ||
r_ret = Variant(); | ||
return ERR_PARSE_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.
Yeah just tested now, I confirm it resets the value to Variant()
(aka null
in GDScript), all assertions in #40795 pass!
This is the case where I'm not really sure whether returning a "half-way" token even on parse error is intended, but I'm not sure if this "bug" was used as a "feature" before. 🙂
2274285
to
a04ef3a
Compare
Additionally reset parse result if error was found.
a04ef3a
to
a2676ff
Compare
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 think we should merge this only in master
for now, and then cherry-pick this to the 3.x
branch after 3.3 is released to avoid introducing regressions in 3.3.
Thanks! |
Cherry-picked for 3.4. |
Related issue: #40794
Tested with:
Some test cases could probably be added to the test added in #40795
Result:
Testing same test cases with macOS's
JSONSerializer
produces same success/failure results.Bugsquad edit: Fixes #40794.