-
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
Add support for preferences schema provided by a plug-in in a manifest file #3159
Conversation
props.push(property); | ||
} | ||
} | ||
this.validateFunction = new Ajv().compile(this.combinedSchema); |
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.
please refactor that it is called only once from the constructor, not for each contribution
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.
Thanks for the review!
My point here is to be able to validate a plugin schema too. Do we have another way to validate it?
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.
sure, you can call it from setSchema
as well
just refactor it in a way that it is called once from the constructor and on each call of setSchema
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.
It is already called once from the constructor since setSchema
is called there. Or maybe I missed something?
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.
it is called in forEach
loop for each contribution
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.
thanks, got it! I'll refactor this
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.
fixed
@@ -96,6 +85,26 @@ export class PreferenceSchemaProvider extends PreferenceProvider { | |||
return this.preferences; | |||
} | |||
|
|||
setSchema(schema: PreferenceSchema, updateValidation: boolean = true): void { |
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.
How abou, instead of adding internal options to public APIs extract 2 methods:
setSchema(schema: PreferenceSchema): void {
this.doSetSchema(schema);
this.updateValidate();
}
protected doSetSchema() ... // updates combined schema
protected updateValidate() ... // updates validate function
and call doSetSchema
and updateValidate
from the constructor instead of setSchema
?
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.
yes, this will be better, I'm updating the PR
and consolidate it with existing ones Signed-off-by: Oleksii Kurinnyi <okurinny@redhat.com>
…in init Signed-off-by: Yevhen Vydolob <yvydolob@redhat.com>
fixes eclipse-che/che#9990