Fixed LSP Linting Synchronization Issues #14685
Conversation
| @@ -335,16 +335,21 @@ define(function (require, exports, module) { | |||
|
|
|||
| function LintingProvider() { | |||
| this._results = new Map(); | |||
| this._defferedLintingPromise = new Map(); | |||
There was a problem hiding this comment.
Nit: the member variables have issues:
- deffered => deferred (typo)
- change defferedLintingPromise to something which seems more intuitive for a map (suggestion: use 'promises' or 'promiseMap' in the name)
- V => v (lowercase, only Modules and Classes are capitalized by convention)
- lintingName => could be changed to 'providerName' or 'linterName'
| this._defferedLintingPromise.get(filePath).resolve(this._results.get(filePath)); | ||
| this._defferedLintingPromise.delete(filePath); | ||
| } | ||
| if (this._ValidateOnType) { |
There was a problem hiding this comment.
Few issues:
- Check if filePath === currentActiveDocumentPath? No need to run inspection for files which are not active.
- How is onSave being handled here?
- Why do we need a promiseMap here at all? The request is being run once we have already gotten the result.
There was a problem hiding this comment.
@shubhsnov on File Save and File Open diagnostics would be run by CodeInspection Manager itself.
in case of file open Diagnostics result won't be available immediately in that case result will be return using promise
There was a problem hiding this comment.
Okay. One more question, if the promise is only for the file open scenario then why do we require a Map? We can just keep a promise for the current doc since we are already not running the code inspection request for any inactive docs.
There was a problem hiding this comment.
FileOpen scenario is the the sceanrio which I could think where result might not be available. there could be other scenarios as well.
There was a problem hiding this comment.
In any case, we only show the linting for the current file. Only having currentActiveDocPromise should suffice. No point in caching since the cached result will never be used.
There was a problem hiding this comment.
Also please add a check in the getInspectionResultsAsync for filePath === currentActiveDocumentPath as well.
already fixed
There was a problem hiding this comment.
Also please add a check in the getInspectionResultsAsync for filePath === currentActiveDocumentPath as well.
already fixed
I specifically meant for getInspectionResultsAsync not setInspectionResultsAsync. It would act as a guard and optimization. Suppose a file has hundred of errors, but we switched to any other file before actually getting the results. Why would we want those hundred of results to pass through another function callback and then get discarded, since the active file changed. Doesn't seem proper to me. If we have a check in set, we should have a check in get.
There was a problem hiding this comment.
@shubhsnov Not Sure if you are looking at complete code workflow from CodeInspection side. It looks perfectly ok to me. the guard you are mentioning is already in place in codeInspection. CodeInspection always call getInspectionResultsAsync for current File only.
and this function can't return null, It has to return promise.
If still not clear we can discuss this.
There was a problem hiding this comment.
It's an optimization. Okay let me explain it to you more clearly here:
You open File A (Some large file which let's assume will take some time for the diagnostics response)
getInspectionResultsAsync will get called
Just as the function was called you switched the document
Now you are inside the getInspectionResultsAsync for File A but the active Document is File B
Now according to me, instead of creating a promise and waiting for the promise to resolve, we can instead do $.Deferred().resolve(null) because it won't matter, as CodeInspection will drop the promise as it is.
There was a problem hiding this comment.
not taking this change as I am not convinced that it is an optimization. we are just double checking if the file is current file.
shubhsnov
changed the title
| @@ -107,7 +107,7 @@ define(function (require, exports, module) { | |||
| CodeHintManager.registerHintProvider(chProvider, ["php"], 0); | |||
| ParameterHintManager.registerHintProvider(phProvider, ["php"], 0); | |||
| CodeInspection.register(["php"], { | |||
| name: Strings.PHP_DIAGNOSTICS, | |||
There was a problem hiding this comment.
Have something here to differentiate from any other provider which might also provide "php" related linting.
There was a problem hiding this comment.
As discussed we are not having any name for default PHP Lint Provider
|
LGTM 👍 |
@shubhsnov please review.
@narayani28