Fix EZP-21666: refactor PromiseCAPI and PromiseService classes #24

wiseman17 · 2013-11-06T12:29:37Z

JIRA issue: https://jira.ez.no/browse/EZP-21666
Classes are fixed and refactored. Tests are on the way...

ezrobot · 2013-11-06T12:31:04Z

jshint with our configuration reports the following issues:

test/manual/jsRestClientBundle/Resources/public/js/cookbook-PromiseCAPI.js: line 8, col 13, Expected 'logRequests' to have an indentation at 5 instead at 13. (W015)
test/manual/jsRestClientBundle/Resources/public/js/cookbook-PromiseCAPI.js: line 9, col 9, Expected '}' to have an indentation at 1 instead at 9. (W015)
test/manual/jsRestClientBundle/Resources/public/js/cookbook-PromiseCAPI.js: line 12, col 5, Expected 'promiseContentService' to have an indentation at 1 instead at 5. (W015)
test/manual/jsRestClientBundle/Resources/public/js/cookbook-PromiseCAPI.js: line 13, col 81, Expected an assignment or function call and instead saw an expression. (W030)
test/manual/jsRestClientBundle/Resources/public/js/cookbook-PromiseCAPI.js: line 4, col 18, 'eZ' is not defined. (W117)
test/manual/jsRestClientBundle/Resources/public/js/cookbook-PromiseCAPI.js: line 6, col 13, 'eZ' is not defined. (W117)
test/manual/jsRestClientBundle/Resources/public/js/cookbook-PromiseCAPI.js: line 11, col 25, 'eZ' is not defined. (W117)
test/manual/jsRestClientBundle/Resources/public/js/cookbook-PromiseCAPI.js: line 12, col 5, 'promiseContentService' is not defined. (W117)
test/manual/jsRestClientBundle/Resources/public/js/cookbook-PromiseCAPI.js: line 13, col 5, 'promise' is not defined. (W117)
test/manual/jsRestClientBundle/Resources/public/js/cookbook-PromiseCAPI.js: line 13, col 15, 'promiseContentService' is not defined. (W117)
test/manual/jsRestClientBundle/Resources/public/js/cookbook-PromiseCAPI.js: line 15, col 1, 'promise' is not defined. (W117)
test/manual/jsRestClientBundle/Resources/public/js/cookbook-PromiseCAPI.js: line 1, col 49, 'CAPI' is defined but never used. (W098)
test/manual/jsRestClientBundle/Resources/public/js/cookbook-PromiseCAPI.js: line 1, col 49, 'SessionAuthAgent' is defined but never used. (W098)
test/manual/jsRestClientBundle/Resources/public/js/cookbook-PromiseCAPI.js: line 1, col 49, 'PromiseCAPI' is defined but never used. (W098)

14 errors

smallish cookbook fixes

Silencing q false positive warning on rejection handling, CS cookbook fixes

dpobel · 2013-11-07T08:32:13Z

src/PromiseCAPI.js

         *
-         * @method generatePromiseService
-         * @param serviceFactory {function} function which returns one of the CAPI services
+         * @method getPromiseService


_getPromiseService
and it should be marked as protected

wiseman17 · 2013-11-07T13:49:54Z

do we really need this? it seems like a big dirty workaround to me

Workaround as opposed to what? Without this option Q library is throwing mentioned warnings whenever we are handling rejection cases (e.g. in real life - any error of any service).

dpobel · 2013-11-07T14:44:15Z

as opposed to:

verify that this line does not actually hide a useful information for the user of the REST Client (ie if he handles the error, the warning is gone)
verify that the problem is not in our code and then fix the code not the consequences

And if it turns out that we really need to call this, please add this in a dedicated issue/commit with a proper commit log and with a proper inline comment because:

that's not a refactoring, that's a new code
given the diff of this PR, I'm pretty sure the same occurs before this patch
referencing a confusing question on stackoverflow is not a good comment

dpobel · 2013-11-07T14:45:54Z

and can you please create a gist with a valid code that allows to get this warning.

wiseman17 · 2013-11-07T15:28:16Z

as opposed to: ...

Both points are exactly what I have done before this fix. But, indeed, it should be done in more "systematic" way.

Should those gists be "executable"? Or just give a straight vision of the problem, like these 2:

This code does NOT throw the warning: https://gist.github.com/wiseman17/7356346
And this chunk does throw it: https://gist.github.com/wiseman17/7356371
The only difference basically is in line 14 - callback behavior.

It will take more time to make "executable" versions, but I can do that as well.

dpobel · 2013-11-07T16:18:33Z

yeah, I'm more interested in a real usage of the REST client than in a excerpt of the unit tests.

wiseman17 · 2013-11-08T11:16:59Z

Here is the "real life" one: https://gist.github.com/wiseman17/7369604

Took some time, as it turned out that at least on Win7 the Chrome is NOT throwing this warning, but FF and IE do throw it all right :).

Fixes according to review

wiseman17 · 2013-11-11T15:24:03Z

Mentioned issues are fixed.
Indeed we can compare against pure key variable which simplifies things a bit.

dpobel · 2013-11-12T08:23:48Z

so I guess, the double name declaration of the method name is not needed anymore ( myClass.prototype.myFunc = function myFunc() {}; can become myClass.prototype.myFunc = function () {};) ?

wiseman17 · 2013-11-12T08:26:08Z

That's true. Should I fix it in this on in a separate issue?

dpobel · 2013-11-12T08:31:03Z

Since that's related to a change done in this PR, that should definitively be done here as well.

Fixes according to review

wiseman17 · 2013-11-12T12:48:54Z

Done.

dpobel · 2013-11-12T12:59:07Z

src/services/ContentTypeService.js

@@ -531,7 +531,7 @@ define(["structures/ContentTypeGroupInputStruct", "structures/ContentTypeCreateS
     * @param callback {Function} callback executed after performing the request (see
     *  {{#crossLink "ContentTypeService"}}Note on the callbacks usage{{/crossLink}} for more info)
     */
-    ContentTypeService.prototype.updateContentTypeDraftMetadata = function updateContentTypeDraftMetadata(
+    ContentTypeService.prototype.updateContentTypeDraftMetadata = function (


the line change for the parameters is not needed anymore. And the same applies for the loadContentTypeGroupByIdentifier and updateContentTypeDraftMetadata methods.

dpobel · 2013-11-12T13:23:01Z

besides my last inline comments, +1

dpobel · 2013-11-12T13:24:42Z

dist/CAPI.js

-                this[key] = this.generatePromiseService(this._capi[key]);
+        for (key in CAPI) {
+            if ( (typeof CAPI[key] === "function") && (/^(get[^\s(]+Service)/).test(key) ) {
+                console.log(key);


console.log !

Fixes according to review

wiseman17 · 2013-11-12T15:41:50Z

Merged

Fix EZP-21666: refactor PromiseCAPI and PromiseService classes

c1f89fb

wiseman17 added 2 commits November 6, 2013 17:39

Fix EZP-21666: refactor PromiseCAPI and PromiseService classes

3853169

smallish cookbook fixes

Fix EZP-21666: refactor PromiseCAPI and PromiseService classes

4edbec1

Silencing q false positive warning on rejection handling, CS cookbook fixes

dpobel reviewed Nov 7, 2013
View reviewed changes

Fix EZP-21666: refactor PromiseCAPI and PromiseService classes

c0e1b80

Fixes according to review

Fix EZP-21666: refactor PromiseCAPI and PromiseService classes

9a31912

Fixes according to review

dpobel reviewed Nov 12, 2013
View reviewed changes

Fix EZP-21666: refactor PromiseCAPI and PromiseService classes

2feee6d

Fixes according to review

wiseman17 closed this Nov 12, 2013

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix EZP-21666: refactor PromiseCAPI and PromiseService classes #24

Fix EZP-21666: refactor PromiseCAPI and PromiseService classes #24

wiseman17 commented Nov 6, 2013

ezrobot commented Nov 6, 2013

dpobel Nov 7, 2013

wiseman17 commented Nov 7, 2013

dpobel commented Nov 7, 2013

dpobel commented Nov 7, 2013

wiseman17 commented Nov 7, 2013

dpobel commented Nov 7, 2013

wiseman17 commented Nov 8, 2013

wiseman17 commented Nov 11, 2013

dpobel commented Nov 12, 2013

wiseman17 commented Nov 12, 2013

dpobel commented Nov 12, 2013

wiseman17 commented Nov 12, 2013

dpobel Nov 12, 2013

dpobel commented Nov 12, 2013

dpobel Nov 12, 2013

wiseman17 commented Nov 12, 2013

Fix EZP-21666: refactor PromiseCAPI and PromiseService classes #24

Fix EZP-21666: refactor PromiseCAPI and PromiseService classes #24

Conversation

wiseman17 commented Nov 6, 2013

ezrobot commented Nov 6, 2013

dpobel Nov 7, 2013

Choose a reason for hiding this comment

wiseman17 commented Nov 7, 2013

dpobel commented Nov 7, 2013

dpobel commented Nov 7, 2013

wiseman17 commented Nov 7, 2013

dpobel commented Nov 7, 2013

wiseman17 commented Nov 8, 2013

wiseman17 commented Nov 11, 2013

dpobel commented Nov 12, 2013

wiseman17 commented Nov 12, 2013

dpobel commented Nov 12, 2013

wiseman17 commented Nov 12, 2013

dpobel Nov 12, 2013

Choose a reason for hiding this comment

dpobel commented Nov 12, 2013

dpobel Nov 12, 2013

Choose a reason for hiding this comment

wiseman17 commented Nov 12, 2013