Skip to content
This repository was archived by the owner on Sep 6, 2021. It is now read-only.

[REVIEW ONLY] Bare async provider implementation.#6375

Closed
busykai wants to merge 1 commit intoadobe:irichter/code-inspection-improvementsfrom
busykai:fix-5137
Closed

[REVIEW ONLY] Bare async provider implementation.#6375
busykai wants to merge 1 commit intoadobe:irichter/code-inspection-improvementsfrom
busykai:fix-5137

Conversation

@busykai
Copy link
Contributor

@busykai busykai commented Jan 6, 2014

@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.

Not working due to an issue in code inspection implementation (html
template is displayed before it's rendered).
@fdecampredon
Copy link

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)

@busykai
Copy link
Contributor Author

busykai commented Jan 6, 2014

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.

@busykai
Copy link
Contributor Author

busykai commented Jan 6, 2014

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

@ghost ghost assigned couzteau and ingorichter Jan 6, 2014
@couzteau
Copy link
Member

couzteau commented Jan 6, 2014

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants