Skip to content
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

Fix: vs-2017.3.host.json: remove allOfand title #2925

Conversation

EmilyGraceSeville7cf
Copy link
Contributor

  • allOf is redundant with one alternative
  • title is redundant with existing title in a referenced schema

@EmilyGraceSeville7cf EmilyGraceSeville7cf changed the title Fix: remove allOfand title Fix: vs-2017.3.host.json: remove allOfand title Apr 25, 2023
@EmilyGraceSeville7cf
Copy link
Contributor Author

EmilyGraceSeville7cf commented Apr 25, 2023

@madskristensen, @GerryFerdinandus, @hyperupcall what's going on? Why CI requests to remove $schema property here and then complains about its removal? markdownlint.json for instance, by this logic should not have CI passed.

@hyperupcall
Copy link
Member

hyperupcall commented May 27, 2023

@EmilySeville7cfg The purpose of the check is to remove duplication and make things more DRY. When specifying a remote schema with ref, adding $schema is duplicated and it is possible for the reference schema to have a different schema. So we enforce that those files must not contain $schema. (when this check was introduced, it fixed issues with a few existing schemas)

If the schema is declared locally, then we want the $schema to be declared since there are no duplication occurring.

@EmilyGraceSeville7cf
Copy link
Contributor Author

So, the schema is referenced to should not have schema key?

@hyperupcall
Copy link
Member

Yup

@EmilyGraceSeville7cf
Copy link
Contributor Author

@hyperupcall, vs-2017.3.host.json refers to src/schemas/json/ide.host.json which has $schema. It's impossible to remove $schema from src/schemas/json/ide.host.json. So does it mean that this PR is invalid?

As a side question: why such check for $schema exists if it can be avoided via allOf?

@EmilyGraceSeville7cf
Copy link
Contributor Author

PR was closed to pay attention for it.

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.

2 participants