-
Notifications
You must be signed in to change notification settings - Fork 257
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
feat: disable default props #606
feat: disable default props #606
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A small improvement to the test cases but otherwise LGTM. Is there a corresponding PR needed for the vscode-yaml repo?
properties: { | ||
scripts: { | ||
type: 'object', | ||
properties: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we possibly have a second non-string property added to this test case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sorry for the longer delay...
I modified the UT. Have a look if I understood correctly your note, please.
vscode-yaml PR is here redhat-developer/vscode-yaml#667
properties: { | ||
scripts: { | ||
type: 'object', | ||
properties: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we have a non-string property added?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks ready to go. I am not sure why CI is unhappy with the new test though
What does this PR do?
Add config to exclude not required default props from completion.
fix: required prop with default value or with const
The reason why users appreciated this feature is that users don't have to specify default properties. Only if the default value needs to be changed, it makes sense to add them (but not all of them automatically).
What issues does this PR fix or reference?
no ref
Is it tested? How?
add unit tests