Skip to content

Conversation

@rchl
Copy link
Member

@rchl rchl commented Mar 12, 2020

  • update vscode-json-languageserver to version 1.2.3
  • update schemas.py with latest updates from http://schemastore.org/api/json/catalog.json
  • add palette command for opening LSP-json settings
  • run server from cache directory

Of course will wait with integrating until lsp_utils is released.

@rchl rchl requested a review from predragnikolic March 12, 2020 21:15
@rchl
Copy link
Member Author

rchl commented Mar 12, 2020

I would also like to flatten options as I did for LSP-eslint to allow for overriding only individual options and make it easier to understand. DONE

@rchl
Copy link
Member Author

rchl commented Mar 12, 2020

Since I just saw this https://github.com/sublimelsp/LSP-json/pull/6/files#diff-0f49ebf0103c2d85a2b61d6712cad9c3 , I guess we should better support languages in an array form.

Copy link
Member

@predragnikolic predragnikolic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will merge this as soon as the lsp_utils is merged to package control

@jfcherng
Copy link
Contributor

jfcherng commented Mar 14, 2020

vscode-json-languageserver marks trailing commas in jsonc as warning since 1.2.2 (this commit) and it looks like it's hard-coded. I wonder there is a way to "ignore" this (for ST's config files, at least).

image

@rchl
Copy link
Member Author

rchl commented Mar 14, 2020

But then the latest VSCode doesn't report trailing commas in its configuration files. Should probably investigate why is that.

@jfcherng
Copy link
Contributor

jfcherng commented Mar 14, 2020

  1. I see a suspicious commit: vscode-langservers/vscode-json-languageserver@c87633b
    But there is actually no code change. So it looks like it's related to its dependency: vscode-json-languageservice
  2. I then check vscode-json-languageservice's changelog . Notice that in 3.3.3: Schemas can configure whether comments and/or trailing commas are permitted.
  3. So it looks like it's related to JSON schema: https://github.com/microsoft/vscode-json-languageservice/blob/289550209598e45e5c483f8e71a9bab262fb486d/src/jsonSchema.ts#L67 but I am not familiar with its usage.
  4. I wonder they do it in a per extension way... microsoft/vscode@bcf67c8. So we have to create a schema file for ST things and use it in this LSP?

@rchl
Copy link
Member Author

rchl commented Mar 14, 2020

Thanks for that. I've looked further from here and came up with this:

  • vscode registers internal schemas and exposes them as URLs. For example vscode://schemas/keybindings for keybindings [1]
  • LSP client (vscode) sends a json/schemaAssociations notification to json server with filePattern => schema mappings:
Sending notification 'json/schemaAssociations'.
Params: {
    "vscode://defaultsettings/keybindings.json": [
        "vscode://schemas/keybindings"
    ],
    "/User/keybindings.json": [
        "vscode://schemas/keybindings"
    ],
    ...
  • json server resolves the schema by loading and parsing it and uses when validating file

Not sure how we could do something similar for Sublime since URL of the schema should ideally be resolvable from a local machine rather than the Internet. If we make it resolve from the Internet then it would be easier (all schemas already included in LSP-json are resolved from the Internet already).

EDIT: Actually we can just copy those schemas to cache directory like we do with the server itself and pass file:// URIs.

[1] https://github.com/microsoft/vscode/blob/839383dc1d843d8dd94823c5f1c99da7500bc1ee/src/vs/workbench/services/keybinding/browser/keybindingService.ts#L678-L727

@rchl
Copy link
Member Author

rchl commented Mar 14, 2020

And as far as I can see in the code, the trick with the schema will work regardless if languageId is json or jsonc. So nothing would be needed from SublimeLSP side.

@rchl
Copy link
Member Author

rchl commented Mar 15, 2020

Actually readme of the server describes this pretty well - https://github.com/vscode-langservers/vscode-json-languageserver

What protocols server fetches itself can be configured with a handledSchemaProtocols option. Schemas that it can't handle (like vscode://, it will request from the client through vscode/content message).

@rchl
Copy link
Member Author

rchl commented Mar 16, 2020

  • added script to update schema list
  • replaced schemas.py with schemas.json that is loaded once on the first retrieve of configuration.
  • removed duplicate configuration options from plugin.py (those are read from sublime-settings).

@rchl rchl requested a review from predragnikolic March 16, 2020 21:54
@rchl
Copy link
Member Author

rchl commented Mar 16, 2020

Went all-in and added schemes for SublimeText-specific files. All with the exception of sublime-keymap contain bare-minimal to not complain about trailing commas and comments. The sublime-keymap schema also contains type definitions for things that are allowed (although not 100% complete).

Copy link
Member

@predragnikolic predragnikolic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left some comments, but this PR is really great. :)

@@ -0,0 +1,6 @@
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also what is the purpose of this file? (tried googling it, but found nothing on the first glance) :)

Copy link
Member Author

@rchl rchl Mar 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's just to enable comments and trailing commas in project-wide vscode configuration files. Not that relevant for Sublime but I'm working on projects where developers split between vscode and sublime so those are common for me at least.

},
{
"fileMatch": ["tsconfig.json", "tsconfig.*.json", "tsconfig-*.json"],
"url": "sublime://schemas/tsconfig"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that you don't need "sublime://schemas/tsconfig" and "sublime://schemas/jsconfig" because they are already defined in schemas.json file :)

Copy link
Member Author

@rchl rchl Mar 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ones that are in schemas.json are official specs that abide schema drafts while those here are additional augmentations for those official ones that contain non-standard properties (vscode extensions). To allow comments, trailing commas and a couple other things you have to use those non-standard properties.
It's ok to provide more than one schema for same file pattern - language server merges those together.

The tsconfig is a file I use a lot in my projects so I'd like it to work well. I'm open to adding more that others find important (for example there are various properties that can be added to package.json since many npm packages allow storing their settings there). :)

@@ -0,0 +1,25 @@
#!/usr/bin/env python3

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think of creating a TextCommand that could be triggered from the command palette. Example LSP-json: Update schemas and that command would run this code bellow?

This can be an improvement for later, if you want to.
But am happy that you took the time to create a script :)

Copy link
Member Author

@rchl rchl Mar 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think they change often enough for it to be worth it. Also, since the package is compressed, we would have to save to cache folder. And also we would need to add dependency on requests package. But my main argument is that I think it's not worth it.

@jfcherng
Copy link
Contributor

jfcherng commented Mar 17, 2020

image

Got this when editing a .sublime-commands file.
https://github.com/sublimelsp/LSP-json/pull/8/files#diff-8bf8f2319ce718821103387c77020004R6
which should be array.

Some other ST files may suffer as well.

rchl added 2 commits March 18, 2020 10:46
 - update `vscode-json-languageserver` to version 1.2.3
 - update schemas.py with latest updates from http://schemastore.org/api/json/catalog.json
 - add palette command for opening LSP-json settings
 - run server from cache directory
@rchl rchl force-pushed the feature/update-server branch from 81fa2ad to cc1a6b3 Compare March 18, 2020 09:50
rchl added 2 commits March 18, 2020 12:22
…aded

 - add a script for updating default schemas
 - save schemas into json file, removing all unused properties
 - load schemas using ResourcePath instead of import py file
Those will be read from default configuration file.
@rchl rchl force-pushed the feature/update-server branch from 1dc0640 to 91b46dd Compare March 18, 2020 11:22
@rchl rchl merged commit 59f4811 into master Mar 19, 2020
@rchl rchl deleted the feature/update-server branch March 19, 2020 08:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants