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

task: make TaskDefinition required optional #10015

Merged
merged 1 commit into from
Sep 8, 2021
Merged

Conversation

vince-fugnitto
Copy link
Member

@vince-fugnitto vince-fugnitto commented Aug 31, 2021

What it does

Fixes: #10014

The commit makes TaskDefinition.properties.required optional based on the json schema validation definition. Previously, if an extension defines a schema that omits the required property the application will throw errors and fail to display the menu. The change treats the missing required property as an empty array as per the spec.

How to test

Review checklist

Reminder for reviewers

image

Signed-off-by: vince-fugnitto [email protected]

@vince-fugnitto vince-fugnitto added bug bugs found in the application tasks issues related to the task system labels Aug 31, 2021
@vince-fugnitto vince-fugnitto self-assigned this Aug 31, 2021
@vince-fugnitto vince-fugnitto force-pushed the vf/task-require branch 2 times, most recently from ccb1d68 to 5dddbca Compare August 31, 2021 18:26
@paul-marechal paul-marechal added this to the 1.17.1 milestone Aug 31, 2021
@vince-fugnitto vince-fugnitto force-pushed the vf/task-require branch 4 times, most recently from dbed5c4 to fdc232e Compare August 31, 2021 18:58
@paul-marechal paul-marechal modified the milestones: 1.17.1, 1.18.0 Aug 31, 2021
@vince-fugnitto vince-fugnitto force-pushed the vf/task-require branch 2 times, most recently from dcb7ec7 to fc6c317 Compare September 1, 2021 15:43
The commit makes `TaskDefinition.properties.required` optional based on
the json schema validation definition. Previously, if an extension
defines a schema that omits the `required` property the application will
throw errors and fail to display the menu. The change treats the missing
`required` property as an empty array as per the spec.

Signed-off-by: vince-fugnitto <[email protected]>
@vince-fugnitto
Copy link
Member Author

@tsmaeder any objections?

@tsmaeder
Copy link
Contributor

tsmaeder commented Sep 8, 2021

@vince-fugnitto which schema are we using? https://code.visualstudio.com/api/references/contribution-points#contributes.taskDefinitions seems to indicate that the field is required. Do we have real cases of extensions that fail because of this?

Not objecting to be more lenient than the json schema in what we accept, though.

@vince-fugnitto
Copy link
Member Author

@vince-fugnitto which schema are we using? https://code.visualstudio.com/api/references/contribution-points#contributes.taskDefinitions seems to indicate that the field is required. Do we have real cases of extensions that fail because of this?

@tsmaeder I initially started working on the issue after noticing errors in the console when using vscode-docker:

image

I believe a real-world example is ms-azuretools.vscode-docker:

In addition, vscode declares the schema like so, so I thought we should be more robust as not to have such errors:

@tsmaeder
Copy link
Contributor

tsmaeder commented Sep 8, 2021

In addition, vscode declares the schema like so

So the documentation is lying ;-) Merge away!

Copy link
Member

@paul-marechal paul-marechal left a comment

Choose a reason for hiding this comment

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

LGTM code-wise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug bugs found in the application tasks issues related to the task system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

task: handle possibly missing required property
4 participants