fredsa / playn

Cross platform game library for N≥4 platforms
0 stars 1 forks source link

Code review request: JSON rewrite #113

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago
Purpose of code changes on this branch:

This code review request is for my big JSON rewrite. I've gutted the two 
existing Java JSON implementations and replaced them with a single 
implementation based on my fast nanojson project. 

Summary of changes:

 - In Java mode, moving from json.org's parser to a more sane parser that matches the behavior of web mode's JSON.parse.

 - Android and flash modes also use the new Java parser (we could potentially share the HTML implementation with Flash, but this is a future project)

 - The JSON html code supports development mode (mainly for unit testing)

 - JSON arrays and objects are now mutable (add, set, remove items)

 - getNumber now returns a float, rather than a double to better match other PlayN APIs

 - Json.Array.getKeys() is now keys() to better match the PlayN API style

 - Add an optional default parameter to all JSON getters (0 or null is the default if you use the single-arg getters)

 - Add type introspection to JSON objects and arrays: isArray, isBoolean, isNumber, etc.

 - Add unit tests for web and java modes

 - All of the methods are now JSON value type-safe. For instance, reading a number as a string will return the default value, rather than throwing an exception. We no longer coerse strings to numbers and vice-versa. 

 - Laid the groundwork for eventually creating a playn-server backend that can share the Java JSON implementation. Writer is now based on nanojson's JsonSink, which can be used in a future server-side implementation to write JSON to HTTP responses

When reviewing my code changes, please focus on:

Note: Google Code's reviewer doesn't show the entire changeset, so I recommend 
using git diff --diff-filter=AMRC to view the entire changeset.

This new package contains the entirety of the new Java parser/writer code, 
ported from my nanojson project:

http://code.google.com/r/mmastrac-playn/source/browse/?name=json-rewrite#git%2Fc
ore%2Fsrc%2Fplayn%2Fcore%2Fjson

This is the implementation on the HTML side:

http://code.google.com/r/mmastrac-playn/source/browse/html/src/playn/html/HtmlJs
on.java?name=json-rewrite

After the review, I'll merge this branch into:
/trunk

Original issue reported on code.google.com by mmast...@gmail.com on 1 Jan 2012 at 8:49

GoogleCodeExporter commented 9 years ago
This is the git changeset for the change in my json-rewrite branch:

http://code.google.com/r/mmastrac-playn/source/detail?r=7088365ccfa4fe16033e0b2d
b3ea1aa1560d6a1a&name=json-rewrite

Original comment by mmast...@gmail.com on 1 Jan 2012 at 8:50

GoogleCodeExporter commented 9 years ago
One additional note to be aware of - the Writer interface has changed enough 
that most consumers of that interface are going to require some changes. The 
biggest difference is that there is no longer a key() method - the key is now 
provided along with the value(), array(), or object(). 

Original comment by mmast...@gmail.com on 1 Jan 2012 at 10:35

GoogleCodeExporter commented 9 years ago
I LGTMed this (modulo comments) over on the changeset page, but I'll repeat 
myself here for posterity.

Original comment by samskiv...@gmail.com on 7 Jan 2012 at 1:04

GoogleCodeExporter commented 9 years ago
Committed to:

http://code.google.com/p/playn/source/detail?r=c27f0afed9091a498f6e1155bf062945e
364a5d5

... with the following changes on top of the patch here to address mdb's 
comments:

http://code.google.com/r/mmastrac-playn/source/detail?r=576ef1f3bc1446fccbd62bb8
54f7f4c61ca4d606&name=json-rewrite

The unittest stuff is a bit of a mess. I'd like to eventually move it to 
playn-core/playn-html, but I'm not sure how we can properly share code between 
all the various platforms in Maven, as it requires adding the tests folders as 
<resources>. 

Original comment by mmast...@gmail.com on 7 Jan 2012 at 11:20