yaml-language-server icon indicating copy to clipboard operation
yaml-language-server copied to clipboard

support specifying schema via $schema key

Open robberphex opened this issue 4 years ago • 26 comments

What does this PR do?

Using $schema key to specify schema

What issues does this PR fix or reference?

Is it tested? How?

CI: Test getSchemaFromFileContent


There is a json file with $schema key:

{
    "$schema": "https://json.schemastore.org/package",
    "name": "name"
}

Auto-completion works based on schema (at VSCode).

But when I convert json to yaml:

$schema: https://json.schemastore.org/package
name: name

Auto-completion doesn't works. So, could yaml-language-server support $schema key? That what this PR do.

robberphex avatar Nov 16 '20 07:11 robberphex

@RobberPhex Thx for the contribution. It would be nice if you fix prettier/eslint errors, to let CI run tests.

evidolob avatar Nov 16 '20 07:11 evidolob

Coverage Status

Coverage increased (+0.08%) to 79.094% when pulling c2596fcc53087a2cb5d1121e1ceaf028796101a7 on RobberPhex:schema-tag into 2bae915c778f74fa464cb838a7b3613f77b8412b on redhat-developer:master.

coveralls avatar Nov 16 '20 08:11 coveralls

@JPinkney Can you look on this PR?

evidolob avatar Nov 19 '20 12:11 evidolob

I won't have time to look at it until next week. But I have a quick question. Since the $schema key is non-standard what happens if a user has a schema with additionalProperties: false? Does the $schema key report an error?

JPinkney avatar Nov 19 '20 12:11 JPinkney

@JPinkney Thx, that's what I thinking about. @RobberPhex yaml-language-server supports comment based alternative for this feature.

evidolob avatar Nov 19 '20 12:11 evidolob

Do you guys have any plan to merge this PR ?

shsuman avatar Feb 18 '21 14:02 shsuman

@shsuman We need to have changes in yaml parser/validator to do not throw error like Property $schema is not allowed. before we can merge this. I can look on it next week.

evidolob avatar Feb 18 '21 14:02 evidolob

Hello @evidolob , Thanks for your reply. We are planning to fork your repo and we would really want this $schema property to work. So, if you could give us a timeline for this feature to be available so that we don't have to do duplicate work

shsuman avatar Feb 18 '21 15:02 shsuman

@shsuman Just out of curiosity is there any particular reason why you would be forking?

cc @gorkem

JPinkney avatar Feb 18 '21 16:02 JPinkney

@shsuman Do not fork or publish the yaml LSP with your own extension such things usually breaks our functionality. We have already had similar cases with different MSFT teams. I think we have an understanding that this is not a good practice.

gorkem avatar Feb 18 '21 17:02 gorkem

@shsuman PRs are welcome.

joshuawilson avatar Feb 18 '21 17:02 joshuawilson

@gorkem Thank you for your response. We are currently in discussion with our team regarding this issue. I will get back to you on the same

shsuman avatar Feb 19 '21 18:02 shsuman

@gorkem Can you briefly describe what problems have you seen in the past with other extensions shipping the Yaml LSP ?

The one thing that we are seeing is duplicate errors and suggestions but other than that we have not observed anything which would either break ours' or yours' functionality.

shsuman avatar Feb 22 '21 19:02 shsuman

@shsuman Our release cycles are not synchronized, you probably are not going to refresh the LSP as often as we do. We end up with getting bug reports for all the YAML issues. Even months after they are fixed.

It is not fair for anybody to expect us to spend time triaging bug reports because somebody have not done the right thing. Therefore, as discussed in this issue, we will warn and even disable functionality in case of conflicts.

gorkem avatar Feb 22 '21 20:02 gorkem

Hey @gorkem!

We appreciate you taking the time to listen to our query. We'd love to provide you with some context on who we are as a team and what we're hoping to achieve with the YAML language server (LS).

We're the Azure Machine Learning (ML) extension team in VS Code. Our extension aims to provide an enhanced developer experience for data scientists and ML engineers targeting the Azure ML service. Many of these users edit YAML configuration files in Code thus we were hoping to be able to provide them with schema validation and completions.

Our plan was to extend your LS to create our own Azure ML language server to provide dynamic completions and preview definitions of cloud resources in addition to the schema-based completions provided by the YAML LS. Currently our completions framework is closely coupled with the LSP validation as we have specific Azure ML tags in our schema which inform when and what completions to surface.

We would love to hear your thoughts regarding the usage and extension of the LSP. Would it be possible for our teams to meet/collaborate to address any concerns you have or talk through alternative approaches? One thing to note is that our customer segment (Azure ML users editing YAML files) is a very small subset of your overall customer base (all VS Code YAML authors) so we don't expect your repository to get flooded with outdated bug reports.

Please let us know if you have any additional questions. We look forward to hearing from you!

Best,

Shantnu Suman on behalf of the Azure ML extension team

shsuman avatar Mar 02 '21 18:03 shsuman

@shsuman Currently we have APIs to provide schemas dynamically that will allow the YAML LS to provide validation code assist etc. We do support a few JSON schema extensions too. I am not sure if those ways would be enough but of course we are open to introducing more extensibility if there are use cases for it.

Are you looking to extend the Yaml LS or vscode-yaml though?

gorkem avatar Mar 02 '21 22:03 gorkem

Hey @gorkem,

Thank you for your reply.

To answer your question, we are trying to extend the Yaml LS and not vscode-yaml.

Also, my team just had a couple of follow up questions: 1. Do you have any documentation on the APIs that you talked about in your last response ? 2. What did you mean by "We do support a few JSON schema extensions too." ?

Lastly, we would highly appreciate it if you would be willing to hop on a call with us to discuss it in detail ? If yes, let me know your preferred contact method as well as your preferred time.

Regards, Shantnu Suman on behalf of the Azure ML extension team

shsuman avatar Mar 04 '21 19:03 shsuman

@gorkem Just a friendly follow up on my last message. Let me know if you have any questions or concerns

shsuman avatar Mar 12 '21 19:03 shsuman

@JPinkney Do you have reference to the extension API and JSON schema extensions that we support. I thought it was a package on npm but I could not find it.

gorkem avatar Mar 16 '21 15:03 gorkem

@shsuman pinged @JPinkney he reminded me where we recorded things take a look at https://github.com/redhat-developer/vscode-yaml/wiki/Extension-API

gorkem avatar Mar 16 '21 16:03 gorkem

@gorkem These API won't prove much useful to us as we are trying to contribute to completions as well as diagnostics dynamically i.e. generated through our extension and not the schemas. For this very purpose, we need to be able to directly contribute to the completions as well as diagnostics returned by the LS

shsuman avatar Mar 23 '21 18:03 shsuman

I do not grok why you would need to run your own copy of the LS, if you are trying to have additional diagnostics or completions. You can register multiple providers for a file.

gorkem avatar Mar 26 '21 12:03 gorkem

@gorkem Because we are putting tags in our schema ("arm_type"), we are using those tags from the schema to determine whether to provide completions and also what completions to provide for a given node

shsuman avatar Mar 26 '21 18:03 shsuman

@evidolob mentioned that this PR may cause validation errors if JSON Schema doesn’t allow additional properties for "root" Object. here https://github.com/redhat-developer/vscode-yaml/issues/567#issuecomment-887248584

As a user I do prefer to have this PR merged and be able to use $schema, with the caveat that until the parser changes are made it might throw on noAdditionalProperties.

safareli avatar Aug 12 '21 07:08 safareli

Still no news on merging this?

carlosedp avatar Aug 03 '23 11:08 carlosedp