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

Added Id Referencing to the toJson function #120

Closed
wants to merge 6 commits into from

Conversation

Projects
None yet
4 participants
@mdavisJr
Copy link
Contributor

mdavisJr commented Jun 22, 2014

Provides the option to use Id Referencing in the toJson function by setting the idAttribute parameter. If idAttribute parameter is left undefined, the toJson function will use JSONPath convention. This allows Dojo's toJSON function to be compatible with JSON.NET.

Added Id Referencing to the toJson function
Provides the option to use Id Referencing in the toJson function by setting the idAttribute parameter.  If idAttribute parameter is left undefined, the toJson function will use JSONPath convention.  This allows Dojo's toJSON function to be compatible with JSON.NET.

@mdavisJr mdavisJr force-pushed the mdavisJr:master branch from 21408ef to de57e1c Nov 26, 2014

mdavisJr added some commits Nov 26, 2014

Modified getRandomId method
Modified getRandomId method in my previous PR.  Changed getRandomId
method to getUniqueId.  getRandomId method was a little overkill don't
really need a random Id(Guid) just an unique Id.  The getUniqueId method
is a simpler.
Fixed whitespace issue
Fixed whitespace issue.

@dylans dylans added this to the 1.11 milestone Mar 25, 2015

@dylans

This comment has been minimized.

Copy link
Member

dylans commented Sep 11, 2015

Thanks for the proposed PR @mdavisRevSpringInc , sorry for the delay in responding to it. As part of our contributor guidelines, we need a CLA ( http://dojofoundation.org/about/cla ) on file. Please let us know once you've completed that. We're looking to get a Dojo 1.11 release done fairly soon, and have listed this as a candidate for that release, but would need to land this patch in the next week or two.

@mdavisJr

This comment has been minimized.

Copy link
Contributor Author

mdavisJr commented Sep 17, 2015

I just submitted my cla. If there are any issues, please let me know.

@wkeese

This comment has been minimized.

Copy link
Member

wkeese commented Sep 17, 2015

It seems strange to check this in without having even a manual test. There's no way to test whether or not the code is working.

@dylans

This comment has been minimized.

Copy link
Member

dylans commented Sep 17, 2015

@wkeese I wasn't going to just blindly check it in, I was waiting for a CLA before reviewing. I think it's useful, so I'll either ask @mdavisRevSpringInc for a test case or I'll write one.

@mdavisJr

This comment has been minimized.

Copy link
Contributor Author

mdavisJr commented Sep 23, 2015

Do you need me to write a test case for this?

@dylans

This comment has been minimized.

Copy link
Member

dylans commented Sep 23, 2015

@mdavisRevSpringInc if you have time that would be great, otherwise I'll plan to add one sometime in the next week or so. https://github.com/dojo/dojo/blob/master/tests/README.md contains info about writing tests... even though most of the dojox tests are done currently with DOH, the tests for dojox/store use Intern ( https://github.com/dojo/dojox/tree/master/store/tests ), so feel free to follow the lead there to do the same. Thanks!

@mdavisJr

This comment has been minimized.

Copy link
Contributor Author

mdavisJr commented Sep 29, 2015

I'm just putting the finishing touches on the tests. They should be ready tomorrow.

Added Tests For dojox\json\ref.js
I've included fromRefJson2 and toAndFromRefJson2 tests to the
dojox\json\test\ref.js file.  I thought about adding the additional test
logic to the original tests but figured it would be safer to create 2
new tests instead of modifying existing tests.

I've also improved the code from my previous commits.

This commit of id based jspon doesn't handle references to arrays but
will handle references of objects inside an array.  I didn't add array
referencing because dojox.json.ref.fromJson will also have to be updated
to handle that format too and I didn't want to rush it in.  We can come
back and add that additional functionality in a future release if it's
needed.

An example of referencing arrays with id based jspon:
var a = [{},{}];
var b = {name:'John', array1:a, array2:a};

converting b to json would look like:
var jsonStr =
'{"$id":"1","name":"John","array1":{"$id":"2","$values":[{},{}]},"Array2":{$ref:"2"}}'

Notice that the array1 becomes an object in json format so that a '$id'
can be assigned to it and then the parser will reverse it back to an
array.

@dylans dylans assigned dylans and unassigned kriszyp Nov 28, 2015

@dylans

This comment has been minimized.

Copy link
Member

dylans commented Dec 10, 2015

Closed via 5e668ac, thanks for the enhancement @mdavisJr !

Note that I allowed a few style guideline inconsistencies in here, because the code in this file is in rather bad shape and really I prefer a more modern style as well. I also noticed a few other minor issues with this file from JSHint. Fixed a couple of them, and added TODO for one that just doesn't make sense how it works currently.

@dylans dylans closed this Dec 10, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.