[CLOSED] [REVIEW ONLY] Bare async provider implementation.

[core-ai-bot]

Issue by busykai
Monday Jan 06, 2014 at 15:38 GMT
Originally opened as adobe/brackets#6375

---

@peterflynn,@ingorichter,@fdecampredon please take at this implementation draft. It introduces an optional scanFileAsync function in the provider interface which when implemented will be used instead of sync scanFile to get the code inspection results. I chose to introduce a separate function since I, personally, don't like multi-semantic functions (e.g. the same scanFile which may return an array or a promise). If there's a compelling argument to overload semantics of scanFile, let me know.

Not working due to an issue in code inspection implementation (html template is displayed before it's rendered). See this comment to #5935. If you want to try it out, just move rendering into the same block where html is prepared (into .then handler).

This is just to get a general agreement on the proposal/approach, a lot of work is missing on the documentation etc. A mergeable PR will be prepared when #5935 is landed in master.

---

busykai included the following code: https://github.com/adobe/brackets/pull/6375/commits

[core-ai-bot]

Comment by fdecampredon
Monday Jan 06, 2014 at 16:14 GMT

---

Like I said on IRC, the problem here is the fact than an inspector could take too much times before resolving or rejecting the promise returned by scanFileAsync.
From what I understand cases where inspectFile is called before the inspectors jobs are finished could occurs.
Similar problematic has already been addressed for code hint (see _updateListHint function of the CodeHintManager).

CodeHintProvider interface has the same method for sync/async case, and check if the returned values are sync result or deferred. You should perhaps do the same here to respect an api logic.

(sorry for my horrible english)

[core-ai-bot]

Comment by busykai
Monday Jan 06, 2014 at 17:23 GMT

---

Wrt to the timing and consecutive calls to scanFileAsync: the provider should implement "reentrant" scanFileAsync function, that is, no state should be stored outside the function scope (at the module level). For example, if the provider does something like in main.js:

define(function (require, exports, module) {
    var results;

    function getResults(text) {
        // get results
        ...
        .then(function (myResults) {
             results = myResults;
        }
    }


    function scanFileAsync(text, fullPath) {
        ...
        getResults(text)
            .then(function() {
                promise.resolve(results);
            }
    }
}

It is plainly the problem of the provider implementation (specifically it is a problem of using module-wide variable to pass data between the functions). If the function scanFileAsync is reentrant and does not have side-effects on the module level, then two consecutive calls to scanFileAsync will be no problem (each will have its own execution context).

Independently, of this "reentrance" issue, a limit should be put on execution time of each scan.

Wrt the same name of the function, I don't see how approach implemented in CodeHintManager.js is any good. Especially, taking into account that linting providers are much more popular (as extensions) compared to code hinting. It is reflected in the comment to this PR too. Having separate function will help to document it and requirements to the implementation (such as the one you brought up) separately. Let's see, however, what core team (@peterflynn, particularly) will have to say.

[core-ai-bot]

Comment by busykai
Monday Jan 06, 2014 at 17:34 GMT

---

By the way, here is an example of JSHint extension ported to this new implementation: busykai/brackets-jshint@df15fb33d . It has significantly simplified the implementation: busykai/brackets-jshint@master...use-async-run

[core-ai-bot]

Comment by couzteau
Monday Jan 06, 2014 at 22:47 GMT

---

I do like the the conceptual clarity seperating the snych and asynch variants of the scanning function. Also performing the scan in parallel makes sense to me.