Fixed issue with multiple levels of json objects

[thomasbellio]

So I am writing an application that needs drag and drop and I wrote a call back with the following signature:

startDrag(event, ui, data){ ..... }

Where data was expected to be a json object. and in my html code I wrote something like this:

onStart:'startCallback(item, {foo: 1, bar: 2})'

This code resulted in a parse error. I pinpointed the issue to be in the callEventCallback function where the code is parsing the arguments based a comma delimited split. This caused problems, because of course the json object is comma delimited, and so my change was to accomodate json arguments to the callback functions. I added 2 test cases, which when applied without my change will clearly illustrate the problem.

Additionally, I used my beautify plugin to format the code, that is why it is showing substantially more changes than are meaningful. I can undo the formatting if that will make things easier, but the important parts are lines 51-83.

[codef0rmer]

Why do not you just store your object in $scope.obj and use it in the dom? On 23-May-2014 3:30 AM, "leftyhitchens" [email protected] wrote:

So I am writing an application that needs drag and drop and I wrote a call back with the following signature:

startDrag(event, ui, data){ ..... }

Where data was expected to be a json object. and in my html code I wrote something like this:

onStart:'startCallback(item, {foo: 1, bar: 2})'

This code resulted in a parse error. I pinpointed the issue to be in the callEventCallback function where the code is parsing the arguments based a comma delimited split. This caused problems, because of course the json object is comma delimited, and so my change was to accomodate json arguments to the callback functions. I added 2 test cases, which when applied without my change will clearly illustrate the problem.

Additionally, I used my beautify plugin to format the code, that is why it is showing substantially more changes than are meaningful. I can undo the formatting if that will make things easier, but the important parts are

lines 51-83.

You can merge this Pull Request by running

git pull https://github.com/RiveraGroup/angular-dragdrop master

Or view, comment on, or merge it at:

https://github.com/codef0rmer/angular-dragdrop/pull/104 Commit Summary

• Fixed issue with multiple levels of json objects

File Changes

• M src/angular-dragdrop.jshttps://github.com/codef0rmer/angular-dragdrop/pull/104/files#diff-0(580)
• M test/spec/tests.jshttps://github.com/codef0rmer/angular-dragdrop/pull/104/files#diff-1(21)

Patch Links:

• https://github.com/codef0rmer/angular-dragdrop/pull/104.patch
• https://github.com/codef0rmer/angular-dragdrop/pull/104.diff

Reply to this email directly or view it on GitHubhttps://github.com/codef0rmer/angular-dragdrop/pull/104 .

[thomasbellio]

The reason I didn't store the object on the controller, is because my use case is somewhat more dynamic. We are using fancytree.js which does not support conventional angular bindings. Additionally the tree is being built when a data set is loaded from the server. When the tree nodes are rendered, a render function is called; and so I have written a service, that I intend to use in several other places to register elements as draggable, dynamically.

The service has a function:

registerDraggable(element, $scope, data);

In this function I added the directives like jqyoui-draggable and specify the callback with the data object as a parameter to the function so that I can broadcast the drag and its data to other controllers.

[thomasbellio]

I realized actually that I missed a case; specifically a multi-level json object such as {r1:1, r2:{ rs1:1, rs: 2}}. This will not work and will still have a parse error. I am working on a more comprehensive parsing strategy and will update the request when it is done.

[thomasbellio]

I have resolved the parsing issue by including jison compile a parser. This way we have a comprehensive way to parse callback args. In the case of json, it treats it as a json object and in the case of plain text just returns the text for compilation. If you want to modify the parser to add cases then you can do so then run the following command:

./node_modules/.bin/jisonjison ./lexersrc/parser.y ./lexersrc/parser.lex -o ./src/Parser.js

This will generate a Parser.js file as deffined in the parser.y grammar.

[srathbun]

Any chance we can get this merged soon?

[codef0rmer]

@leftyhitchens: It seems that tests are failing. Would you mind writing some test cases for your fix and fix the broken ones as I'm little loaded?

[thomasbellio]

I wrote a couple of test cases for the issue that I ran into, they are named as follow;

"should parse json arguments as json", "should parse json objects along with non json objects" "should parse nested json objects as a single argument" "should parse empty arguments"

All of the test cases are passing when I run them locally, and when I do a fresh build. I noticed the Travis CI is failing but I haven't seen one that was successful. Can you point me to the cases that are failing for you?

[codef0rmer]

Thanks @leftyhitchens, I'll check your commit soon. Little loaded, sorry for the delay.

[KylePDavis]

Just to update this, I wanted to let you know that:

1. we found some serious issues with this fix (breaks with literal values as arguments, for example)
2. that we no longer need this (we can just put it on scope like you'd originally suggested)

I work on the same team as @leftyhitchens and @srathbun.

I'll go ahead and delete our fork for this and you can feel free to close out this pull request.

[thomasbellio]

@KylePDavis

Can you elaborate on point 1? I have a test case that specifically tests for a literal argument:

"should parse json objects along with non json objects"

[KylePDavis]

@leftyhitchens That test parses arg1,{"r1":1,"r2":2} but it fails in the case where there are literals in the string, such as: 1, true, false, null, etc. So something like arg1,42,{"r1":1,"r2":2} fails.

This should probably use the built-in AngularJS $parse method which will actually just do all of these things.