sublimelsp / LSP-json

Schema validation/completions for your JSON and Sublime files
MIT License
75 stars 10 forks source link

Implement support for reading schemas from Packages #43

Closed rchl closed 4 years ago

rchl commented 4 years ago

Resolves #18

BTW. Some internal details:

@rwols @predragnikolic @jfcherng @deathaxe

rwols commented 4 years ago

lsp_utils doesn't contain a class called ApiWrapperInterface at my local setup?

rchl commented 4 years ago

Oh, right. I guess I have to ship that first. :)

rchl commented 4 years ago

lsp_utils doesn't contain a class called ApiWrapperInterface at my local setup?

Added in https://github.com/sublimelsp/lsp_utils/pull/21

predragnikolic commented 4 years ago

maybe rename sublime-package-types.json to

  1. settings-schema.json (shorter, but no indication that it has someting to do with LSP) or
  2. lsp-settings-schema.json(a bit longer, but it communicates that this file is related to lsp)
  3. rafalint.json (maybe we can alias it to rchlint.json) :)
rchl commented 4 years ago

It actually is not specific to LSP. This schema is ST Package specific and any package can provide their own types.

rchl commented 4 years ago

To clarify, any package can define sublime-package-types.json to contribute a settings schema to its own settings file or any other file. In the future that can be extended to allow contributing schema to other *.sublime-foo files. Since this is a LSP-json package, its schema is for the LSP-json.sublime-settings and thus it's LSP-specific.

The sublime-package-types.json doesn't have a nice ring to it but I don't have better suggestions at the moment. I think ideal would be sublime-package.json but as I've mentioned before, that is problematic as it picks up the schema for package.json (which I'd say is a bug in schema store).

predragnikolic commented 4 years ago

Should we just put a / in front of package.json in LSP-json/lsp-json-schemas.json

  {
    "fileMatch": [
-      "package.json"
+      "/package.json"
    ],
    "uri": "https://json.schemastore.org/package"
  },

and go with sublime-package.json? EDIT: but we have a script for pulling new schemas, and when we pull them, they will override this override... so I don't know.

rchl commented 4 years ago

We can change the script to prefix all patterns. I wanted to avoid that but now that I think about it, it's probably fine.

predragnikolic commented 4 years ago

Now I stopped getting completions when inside sublime-package.json file. I think that it is because LSP-json/schemas/sublime-package.json. That file now should be named differently... sublime-package-schema.json and inside that file, the $id should be changed to something else, and the lsp-json-schemas_extra.json:65 should be updated accordingly. :)

That is something that I did to make it work again, but maybe you have a better solution.

rwols commented 4 years ago

I did as the README told me:

1) put a file called sublime-package.json in LSP/ 2) put the example code in it

But the schema for this file is not picked up, because I'm getting error diags:

Schermafbeelding 2020-08-22 om 14 15 24
rwols commented 4 years ago

Ah, it does work. It's just that the example code has comments which are not allowed?

rwols commented 4 years ago

The hover is fighting with PackageDev unfortunately. We need a setting in PackageDev to disable its hover :)

rwols commented 4 years ago

OK, I added "show_view_status" and it seems to work. The rest of the keys I haven't added and it correctly marks them as unknown. Very nice!

Schermafbeelding 2020-08-22 om 14 48 11
rchl commented 4 years ago

Comments should definitively be supported in those schemas. Will look into that.

As for PackageDev, I've added settings to it to disable tooltips and completions but it seems like there was no new version released yet.

rwols commented 4 years ago

The schema is now correctly picked up. markdownDescription doesn't seem to be completed.

rchl commented 4 years ago

markdownDescription and a couple of other properties are VSCode extensions, not part of the official schema, and I don't think they've bothered to create schema for those properties. Here is the list of those: https://github.com/microsoft/vscode-json-languageservice/blob/76f4b43d475f67fd58c6bd2eaea1b0cd01826f84/src/jsonSchema.ts#L56-L68

If there is no schema for those then I guess we can create it ourselves.

rwols commented 4 years ago

It doesn't seem to reload my sublime-package.json file when I do "LSP: Restart Servers". This makes it somewhat annoying to write a sublime-package.json file because I need to restart ST.

rchl commented 4 years ago

Yes, those are cached globally right now. I think it only makes sense to re-read everything when developing so maybe there should be a setting for doing that.

rwols commented 4 years ago

I am kind of on the fence of this markdownDescription key. If SublimeHQ were to adopt the sublime-package.json file for their own, they would be forced to write a markdown parser too. Furthermore, if we're going to write some tooling around converting existing .sublime-settings files and their comments to a more rigid sublime-package.json, then we are also forced to populate description instead of markdownDescription, because it's not guaranteed that comments are markdown in existing .sublime-settings files.

rwols commented 4 years ago

Interestingly, vscode-json doesn't provide completion docs when it's a markdownDescription, but it does provide completion docs when it's a description.

rchl commented 4 years ago

Do you have any concerns if we ignore SublimeHQ adopting it? Because I think that might be a bit far fetched wish so not sure if we should worry about that.

As for tooling, I think it would require manually reviewing and editing the results anyway so it won't be fully automatic. And the tooling can just choose whether to create description or markdownDescription.

rchl commented 4 years ago

Interestingly, vscode-json doesn't provide completion docs when it's a markdownDescription, but it does provide completion docs when it's a description.

Maybe we need both then. ;)

rwols commented 4 years ago

All valid points. OK, I yield to markdownDescription.

rchl commented 4 years ago

It doesn't seem to reload my sublime-package.json file when I do "LSP: Restart Servers". This makes it somewhat annoying to write a sublime-package.json file because I need to restart ST.

It will now automatically notice changes to schemas, without restart or even re-opening the file.

predragnikolic commented 4 years ago

I do wonder how this works with the dotted syntax? I haven't tried it.

rchl commented 4 years ago

I do wonder how this works with the dotted syntax? I haven't tried it.

I think it will only work with either dotted or not dotted, depending on how you define the schema. Unless you define both variants.

rchl commented 4 years ago

I still need to figure out how to handle fileMatch-less schemas so that we can have one common base schema for all LSP-* packages. The server doesn't seem to want to merge schemas based on $schema, only file patterns.

rchl commented 4 years ago

Of course it's no problem if we assume that all LSP- packages use `LSP-.sublime-settingsconvention. But I want to figure out if it's possible to merge schemas based on$schema` as otherwise, we might hit some limitations in the future.