New pull of changes before rearranging folders

[bouks]

This new pull include :

• automatic detection of data format
• fix bug with prefixed rowid and inline edit
• number helper optimization

[meh-uk]

Can we write some unit tests for this to check it works so everyone is happy with it?

[bouks]

I've never used unit testing in javascript (only in PHP) but i think if we want to do tests we must rewrite code to separate functions. For this example, we should do a function only for this ajax call with only local vars and return only datas and datatype.

Must we do that ?

Le 11/01/2015 23:36, Matthew Hutton a écrit :

Can we write some unit tests for this to check it works?

— Reply to this email directly or view it on GitHub https://github.com/openpsa/grid.js/pull/30#issuecomment-69515351.

[meh-uk]

I was more thinking writing a test with each different data type and data type specification with jquery 1.7 and assert that it spits out the right thing on the page. I suppose it would be more a functional test than a unit test.

You can certainly load json from a local file as I have with the grouping demos.

You and Oleg have had a decent back and forth here, I think something more needs doing so everyone is satisfied. I'm happy to help if you want.

[bouks]

Then maybe we must use phantomJS or SlimerJS.

Note that functionnal testing take much time, we must learn how testing and we are a little team with other projects (jobs).

I can do demos but it is not testing anything and this might overload much the demos, and i've already tested it "like a demo".

Le 11/01/2015 23:53, Matthew Hutton a écrit :

I was more thinking writing a test with each different data type and data type specification with jquery 1.7 and assert that it spits out the right thing on the page. I suppose it would be more a functional test than a unit test.

You can certainly load json from a local file as I have with the grouping demos.

You and Oleg have had a decent back and forth here, I think something more needs doing so everyone is satisfied.

— Reply to this email directly or view it on GitHub https://github.com/openpsa/grid.js/pull/30#issuecomment-69516037.

[meh-uk]

Let's see what I can do tomorrow :).

[flack]

@bouks While the discussion about the code changes is ongoing, if you want, you can submit the changes to grid.locale-fr.js separately. I don't think anyone is going to argue about them :-)

@meh-uk AJAX requests when running from the file system is prohibited by security settings in some browsers, so it's not that easy unfortunately. For example, when I worked on your demos PR, Firefox refused to load the data when running from the HTML file directly. I had to set up a local server and run the demos over HTTP for the AJAX stuff to work

[meh-uk]

@flack - ah I didn't realise that. It sounds like tests will be more effort than they are worth in this case, so I have no objection to merging this as is.

[flack]

P.S.: karma (the test runner I integrated into Grunt for the qunit tests) automatically spawns a server when running tests, see http://karma-runner.github.io/0.12/intro/how-it-works.html

I have never actually tried it, but maybe it is possible to somehow use it to serve the data in unit test runs

[bouks]

Really, i don't know what is going on with my automatic detection.

I've tested it with json and xml, with mime types and not, and it works. You can too, it's not a big thing.

There's no complicated stuff in this commit and i repeat, the "intelligent guess" was implemented years ago.

jQgrid worked and you used it for years without tests.

Programming is not blinded fear.

Le 12/01/2015 10:46, Matthew Hutton a écrit :

@flack https://github.com/flack - ah I didn't realise that. It sounds like tests will be more effort than they are worth in this case, so let's not bother.

— Reply to this email directly or view it on GitHub https://github.com/openpsa/grid.js/pull/30#issuecomment-69546996.

[flack]

Note: We should probably update the documentation (specifically http://openpsa.github.io/grid.js/configuration/options.html#-datatype-) to reflect this change

[bouks]

Ok.

So, according to the change :

• "xml" is not an option anymore (but set it doesn't break anything)
• "json" is not an option anymore (but set it doesn't break anything)

Also :

• it seems that "javascript" is not implemented (in populate function -- has it been implemented once ?)
• it seems that "function" is not implemented (in populate function -- has it been implemented once ?)

[bouks]

We also should remove the "xml" default to "" in the code.

[OlegKi]

The datatype: "jsonp" can be used too. See the answer or this one which loads the data from another server which provides public available data. The answers include demos which still work.

The option "script" instead of "javascript" can be used too (see the lines of jqGrid code).

The usage of datatype as function is also possible (see the line). On should call addJSONData or addXmlData. The kind of usage can be helpful if one have another API as jQuery.ajax to communicate with the server.

It's important to decide which version of jQuery one will to support. I mention in my previous post that jQuery 1.7. And it seems me important don't change and requirements here without any really important reason. I personally use currently jQuery 1.11.2 in all my demos. I don't like jQuery 2.1.3 exactly because of default security restrictions which prevent loading Ajax data from the local files.

Now looked through new suggestion for autodetection and want to comment the code.

I personally have seen demos which uses no url (see the default url: "" are used) and it seems that the Ajax requests to the current URL server will be done. The server code detect loading of static HTML file from the Ajax request and the program seems to work with no url specified. The detection suggested by bouks will not work in the case.

Another problem which I see: the code will make Ajax request to the server even if datatype: "local" are set if non-empty url exists. It will break loadonce: true functionality. So the current changes of the code should clear be not applied.

If would try to implement some kind of autodetection of format of Ajax responses myself I would use dataType: "text" option of jQuery.ajax to load and then parse it after the detection. It's not so simple how to detect format of data more safe. One can use Content-Type header of the server response, the body content and other. Some versions of jQuery are more strong, another less strong. I know the example that jQuery change the behavior of some relatively new version of jQuery so that no empty lines are allowed at the beginning of JSON response.

I answered some time before a answer and have found that the data of jqGrid was not loaded after changing to new jQuery and the reason was the usage of empty line as the beginning of the server respone. So I am very careful and not to force other to upgrade his version of jQuery to the new version.

[flack]

How's about setting the default for datatype to null, and only do an auto-detection in that case?

So the logic would be:

• if datatype is set, use it
• otherwise, see if data is populated. If yes, set datatype to local
• otherwise, send a request against url and try to autodetect from response
• if everything fails, throw an exception with a message like "dataype autodetection failed, please specify it manually"

IMO, it is not so important that autodetection works for all strange corner cases, but for the 90% most common use cases. For everything else, the user can still set datatype manually

[flack]

P.S.: on second thought, datatype: "auto" might be a better wording

[meh-uk]

I like that.

[bouks]

Note that :

• autodetection is effective ONLY if 'url' option is filled.

In that case :

• json and jsonp have the same treatment in our code as in the tony's project.
• if "script", the treatment is identically to json as in the tony's project.

So, no matter if datatype is filled (by user) or not if url is filled.

"It's important to decide which version of jQuery one will to support. "

Autodetection WORKS with jquery 1.7 !

I push a commit to fix the loadonce bug :

https://github.com/bouks/grid.js/commit/21f3592afaae100abb09d4c1f361149ae489b380

then :

• autodetection is effective ONLY if 'url' option is filled and datatype is not 'local'

[OlegKi]

I just posted the comment with the long description of reasons why I changed the default value of datatype from "xml" to "local". I don't imagine any datatype: "auto" which works safe. The only option which don't make any Ajax request to the server is datatype: "local". All other do makes Ajax request.

I've seen a lot of grids where one used many different combination of options. So I don't know any safe method to detect datatype. To comment suggestion of @flack I can remark that reloading of data (refreshing) previously loaded from the server in case of usage loadonce: true have data option filled from the previous request. So the existing of non-empty data option can't be directly used as the criteria of detect datatype. One can modify the code of reloadGrid, but in any way I find the problem of detection of datatype very complex and different to test.

[bouks]

@OlegKi Only "json", "jsonP", "xml" and "script" all others won't make ajax calls : "xmlstring", "jsonstring", "local" and "clientside".

[flack]

@OlegKi reloadGrid shouldn't be a problem, because the autodetection (which runs only when the grid is first constructed) would set datatype according to what it has detected. Or am I missing something?