Fix gotype-live errors showing in wrong file

[tylerstillwater]

There exists an issue with the live error system in that gotype-live will report errors for all files in the package, but the document change event is only for a single file. As a result, all the errors, for all the files, were being displayed as errors in the current file. This PR fixes that issue.

It may be worthwhile to investigate the possibility of displaying those errors for the correct file.

[tylerstillwater]

@ramya-rao-a I've just updated the PR to parse the errors from all the files and add them to the appropriate diagnostic set. This allows the live error system to now clear all errors from the errors diagnostic collection and replace them with the live errors coming from gotype.

[ramya-rao-a]

gotype-live will report errors for all files in the package

So in that case, shouldn't we refrain from clearing the errors for files from other packages?

Also, lets enable the live error feature when files.autoSave != 'afterDelay' || files.autoSaveDelay > liv-error delay *1.5 and listen for feedback on how that feels for users

[tylerstillwater]

Apologies. I misspoke. It will report errors for all files as though you were attempting a build. The messages will be slightly different, but you will get all the same errors (at least in my experience). As such, I believe it is safe to clear all errors and let gotype repopulate them as we go. I'll get that change made also.

…

On Apr 17, 2017, 12:32 AM -0600, Ramya Rao ***@***.***>, wrote: > > gotype-live will report errors for all files in the package > So in that case, shouldn't we refrain from clearing the errors for files from other packages? Also, lets enable the live error feature when files.autoSave != 'afterDelay' || files.autoSaveDelay > liv-error delay *1.5 and listen for feedback on how that feels for users — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub (#923 (comment)), or mute the thread (https://github.com/notifications/unsubscribe-auth/AAEUlBYxYDJhelkmT5V8IYOw11FIFWxEks5rwwdhgaJpZM4M9ARG).

[tylerstillwater]

@ramya-rao-a as requested, live errors now works under the specified autoSave conditions.

Please let me know if you need anything else before merging. Thanks!

[ramya-rao-a]

just curious... why add the prefix file:// before parsing the file to get the uri? vscode.Uri.parse should work with just the file path as well

[tylerstillwater]

When I tried it without the prefix, it didn't work. It would report the errors, and I clicked on one, it would say the file could not be found.

…

On Apr 17, 2017, 11:05 AM -0600, Ramya Rao ***@***.***>, wrote: @ramya-rao-a approved this pull request. In src/goLiveErrors.ts (#923 (comment)): > @@ -20,8 +20,11 @@ export function goLiveErrorsEnabled() { if (goConfig === null || goConfig === undefined || !goConfig.enabled) { return false; } - let autoSave = vscode.workspace.getConfiguration('files')['autoSave']; - if (autoSave !== null && autoSave !== undefined && autoSave !== 'off') { + let files = vscode.workspace.getConfiguration('files'); + let autoSave = files['autoSave']; + let autoSaveDelay = files['autoSaveDelay']; + if (autoSave !== null && autoSave !== undefined && autoSave !== 'afterDelay' || autoSaveDelay < goConfig.delay * 1.5 would be simpler In src/goLiveErrors.ts (#923 (comment)): > }); - errorDiagnosticCollection.set(uri, diagnostics); + diagnosticMap.forEach((diagnostics, file) => { + errorDiagnosticCollection.set(vscode.Uri.parse('file://' + file), diagnostics); just curious... why add the prefix file:// before parsing the file to get the uri? vscode.Uri.parse should work with just the file path as well — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub (#923 (review)), or mute the thread (https://github.com/notifications/unsubscribe-auth/AAEUlOvGdNirxmoNndOXPcBWv-Cn0fkjks5rw5u_gaJpZM4M9ARG).

[ramya-rao-a]

thats interesting, i'll look into that separately.

Thanks for the fix!

I'll combine this one with a few other bug fixes and release in a few days

[tylerstillwater]

Sounds good! Thanks!

[ramya-rao-a]

@tylerb I was testing this in Windows and the errors were not getting mapped to the right file. Probably due to issues with path separators while trying to parse file:\\' + file.

08e9c10 to fix the issue.

Just FYI, in case you came across the fix next time you were looking at this file

[tylerstillwater]

Excellent thanks! I don't have a windows box for testing. Sorry about that!

…

On May 1, 2017, 9:01 PM -0600, Ramya Rao ***@***.***>, wrote: @tylerb (https://github.com/tylerb) I was testing this in Windows and the errors were not getting mapped to the right file. Probably due to issues with path separators while trying to parse file:\\' + file. 08e9c10 (08e9c10) to fix the issue. Just FYI, in case you came across the fix next time you were looking at this file — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub (#923 (comment)), or mute the thread (https://github.com/notifications/unsubscribe-auth/AAEUlM5fhNpbDQu_D-1_5mcZfmD_pZXEks5r1pykgaJpZM4M9ARG).

[ramya-rao-a]

No problem 😊