-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Validate Preferences Used in Editor #10607
Conversation
@vince-fugnitto, would you mind taking a look at this? If desired, I can remove the changes related to JSON schema typing - it was convenient when writing the validation, but I think the only actual problems it detected were in the plugin task system, where we were using the |
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 confirmed that the changes address the problem where invalid preferences values would break the app. The framework is now robust where we validate preferences, and fallback properly when incorrect 👍 I really like the tests as well! :)
packages/core/src/browser/preferences/preference-validation-service.spec.ts
Outdated
Show resolved
Hide resolved
packages/core/src/browser/preferences/preference-validation-service.ts
Outdated
Show resolved
Hide resolved
3b8fea1
to
9f3e33a
Compare
95f6c5d
to
3282d49
Compare
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 confirmed that #10599 is resolved with the latest changes 👍
packages/core/src/browser/preferences/preference-validation-service.ts
Outdated
Show resolved
Hide resolved
packages/core/src/browser/preferences/preference-validation-service.ts
Outdated
Show resolved
Hide resolved
9c14b79
to
3861155
Compare
3861155
to
26743ab
Compare
26743ab
to
b35a49f
Compare
…bugfix/bad-pref-values-in-editor
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.
The changes work well for me 👍
I confirmed that the preferences are now correctly validated (for example in the case they are manually edited), and if a type or value is invalid the preference will default back to it's default value.
I also confirmed that my previous observation of the multiple logs has been fixed and only one validation message is displayed in the console. The marker in the editor is also present.
What it does
Fixes #10599 by adding a service to validate preference values used by Monaco.
Fixes #10678 by modifying the conditions for emitting events and the
affects
method ofPreferenceProxies
It also improves the typing of JSON schemas and fixes some minor issues related to those typings.
How to test
Review checklist
Reminder for reviewers