New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Less distracting error message for JSON schema resolution issues #60219

Merged
merged 10 commits into from Oct 29, 2018

Conversation

Projects
None yet
3 participants
@xxyy
Contributor

xxyy commented Oct 9, 2018

Related issue: #51032

Ellipsis

Instead of showing an error in the Problems tab, this PR changes the JSON Language Service to show a status bar icon if a JSON Schema cannot be resolved. This icon is hidden again once the current file is changed.

2018-10-10_15 10 13

Technical Summary

As suggested here by @aeschli, this adds a middleware that intercepts diagnostics from the server. If one of the diagnostics is a ErrorCode.SchemaResolutionError, it is removed from the diagnostics array and the status bar icon is shown. Further diagnostics (e.g. syntax errors) are left untouched.

Note: To test this, point schemastore.azurewebsites.net to a non-existing IP address (e.g. 10.87.254.254) via a local DNS server or /etc/hosts for a schema resolution error.

Caveats

  • This adds a dependency on vscode-json-languageservice to the client, which might not be desired. However, it is the only apparent way to use the actual enum constant for the error code instead of a magic number. This uses a magic number for the remote event code.
  • When switching back to a JSON file, the error only appears when schema resolution is reattempted. This happens on save.
  • There is currently no way to retry.

Further Notes

No tests are provided. This reason for this is that I was not able to find any tests for that particular module and adding entirely new ones seemed overkill for this issue.

I will squash the commits once I get feedback on whether the new dependency is desired and if further adjustments are needed.

#Hacktoberfest

xxyy added some commits Oct 8, 2018

Show JSON schema resolution errors in status bar only
This commit adds a middleware to the JSON Language Feature client
which intercepts diagnostics for schema resolution errors, and shows
them in the status bar instead of treating them as code errors.

Fixes #51032
Use ErrorCode in JSON Language Service schema resolution error handling
This adds a dependency on vscode-json-languageservice to the client,
which might not be desired, but allows for cleaner code and the correct
reference should the error codes change.
@msftclas

This comment has been minimized.

msftclas commented Oct 9, 2018

CLA assistant check
All CLA requirements met.

@aeschli

This comment has been minimized.

Contributor

aeschli commented Oct 9, 2018

Thanks a lot @xxyy !

I meant something like what TSLint shows:
image

There can be a hover on the status item explaining that validation for the current file is disabled as the schema could not be resolved.
Clicking on it can offer a retry action (that would likely require a new type of message that we send to the JSON server).

@aeschli

This comment has been minimized.

Contributor

aeschli commented Oct 9, 2018

Also we need to improve the error message. That's to be done in the vscode-css-languageservice.

xxyy added some commits Oct 10, 2018

Show JSON schema resolution issues in status bar, not notifications
Note that this uses the status bar item only for that. After switching files,
the item is not shown until problems
are re-evaluated.
@xxyy

This comment has been minimized.

Contributor

xxyy commented Oct 10, 2018

I have changed the error code to use the magic value instead of the dependency. Also, I'm now using a custom status bar item to show the error, as suggested. (see the updated PR description for a screenshot) The item is only shown if a schema error occurred and hidden otherwise.

I have also added the retry action. Below the error message (I'll look into making a PR to make it nicer in vscode-json-languageservice later this week), a hint is shown that one may click to retry. When the retry command is received, the icon changes to a watch and the server is notified to re-verify the current file. It changes back or disappears when the server sends the resulting diagnostics.

Currently, I'm hiding the status bar item when the active editor is changed – it's not relevant in any other file. However, the issue is that diagnostics are not recomputed when switching tabs, so the icon will not reappear unless the JSON file is changed. Should I just keep the status bar icon when switching files? (might be annoying) If not, i'd have to re-verify the schema every time a JSON file is switched to. (If there's no way to have hidden diagnostics)

Also I'm unable to get the locale string from package.nls.json to show in the UI, it always shows the fallback. Do I have to do something else to add a new nls message?

@aeschli

I'd remember which open documents have scheme resolve errors so you can use this when an editor gets active.

@aeschli

This comment has been minimized.

Contributor

aeschli commented Oct 11, 2018

Great work @xxyy !

Currently, I'm hiding the status bar item when the active editor is changed – it's not relevant in any other file.

yes, we should only show the warning when the corresponding editor has focus.

I suggest to have a map with URI to schema-resolve error, so when the editor gets active you use that.
Entries can be removed from the map when a document is closed (onDidCloseTextDocument).

xxyy added some commits Oct 11, 2018

Refactor SchemaRetryNotification to ForceValidateRequest
Now returns the new diagnostics.

Also, actually refresh the schemas instead of just revalidating the
documents. It worked only sometimes before.
@xxyy

This comment has been minimized.

Contributor

xxyy commented Oct 11, 2018

I'm now caching the files with resolution errors. Also, I changed SchemaRetryNotification to ForceValidateRequest, which returns the diagnostics. Also I noticed that the schemas were not getting refreshed every time, which they are now. (All configuration is reloaded as well, which shouldn't be an issue for now, idk if that will change once this endpoint is used for different things)

@aeschli aeschli merged commit 26c3d8d into Microsoft:master Oct 29, 2018

1 of 2 checks passed

VS Code in progress
Details
license/cla All CLA requirements met.
Details
@aeschli

This comment has been minimized.

Contributor

aeschli commented Oct 29, 2018

Thanks @xxyy !

@aeschli aeschli added this to the October 2018 milestone Oct 29, 2018

@xxyy xxyy deleted the xxyy:pn/51032/json-status-bar branch Oct 29, 2018

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