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

Optional schema validation #11189

Merged
merged 1 commit into from
May 24, 2022

Conversation

kittaakos
Copy link
Contributor

What it does

Made the preference schema validation optional.

  • It costs time at startup, and it only logs warnings.
  • Can be disabled with the shouldValidateSchema preference config.

This PR contains a commit with an example. I can drop the example commit from the PR.

How to test

Start the app, and you will see these in the log:

2022-05-21T16:37:49.331Z root INFO Preference schema validation is disabled.
2022-05-21T16:37:49.331Z root INFO Preference schema validation is disabled.
2022-05-21T16:37:49.331Z root INFO Preference schema validation is disabled.
2022-05-21T16:37:49.332Z root INFO Preference schema validation is disabled.
2022-05-21T16:37:49.332Z root INFO [b102b270-04b0-4af7-93f7-d8b1f0e2c9ef][vscode.css-language-features]: Loaded contributions.
2022-05-21T16:37:49.332Z root INFO [b102b270-04b0-4af7-93f7-d8b1f0e2c9ef][vscode.debug-auto-launch]: Loaded contributions.
2022-05-21T16:37:49.333Z root INFO [b102b270-04b0-4af7-93f7-d8b1f0e2c9ef][vscode.debug-server-ready]: Loaded contributions.
2022-05-21T16:37:49.333Z root INFO Preference schema validation is disabled.
2022-05-21T16:37:49.334Z root INFO [b102b270-04b0-4af7-93f7-d8b1f0e2c9ef][vscode.docker]: Loaded contributions.
2022-05-21T16:37:49.334Z root INFO Preference schema validation is disabled.

Please review. Thank you! 🌻

Review checklist

Reminder for reviewers

@msujew
Copy link
Member

msujew commented May 21, 2022

@kittaakos I'm alright with having an option to disable schema validation, but having it behind an overridable method seems overengineered for me. What do you think about a frontend config option instead?

@kittaakos
Copy link
Contributor Author

I'm alright with having an option to disable schema validation

Thank you!

What do you think about a frontend config option instead?

Sure. Do you mean FrontendApplicationConfig?

@msujew
Copy link
Member

msujew commented May 23, 2022

Do you mean FrontendApplicationConfig?

Yes exactly. It allows downstream users to easier disable those validations, since the json can be easily modified during the build process. Having something like this locked behind code basically only allows manual manipulation, which is then app-wide.

@colin-grant-work
Copy link
Contributor

The time savings here are substantial. In my comparison runs, 3.8ms was spent in doSetSchema without validation, 124.4ms with validation. I support @msujew's suggestion that the toggling be moved to FrontendApplicationConfig unless we can imagine a use-case where some condition might be applied rather than a blanket true or false?

@kittaakos kittaakos force-pushed the optional-schema-validation branch 2 times, most recently from 163af76 to bbb2fd1 Compare May 23, 2022 17:42
@kittaakos
Copy link
Contributor Author

Thank you for the feedback. I updated the PR. Please review.

It costs time at startup and it only logs warnings.
Can be disabled with the `validatePreferencesSchema`
fronted app config.

Signed-off-by: Akos Kitta <[email protected]>
@kittaakos
Copy link
Contributor Author

Please review.

Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

The changes look good to me! Don't forget to revert your test commit before merging 👍

@kittaakos
Copy link
Contributor Author

Don't forget to revert your test commit before merging 👍

Do you want me to drop the examples customization?

@msujew
Copy link
Member

msujew commented May 24, 2022

@kittaakos Yes, more specifically 43456de, after all we want to have validations for our example app (for us to able to test that the validation works as expected)

@vince-fugnitto vince-fugnitto added the preferences issues related to preferences label May 24, 2022
@kittaakos
Copy link
Contributor Author

Could you please merge if all looks good? Thank you!

@msujew msujew merged commit fad026f into eclipse-theia:master May 24, 2022
@colin-grant-work
Copy link
Contributor

@kittaakos, it looks like you still have committer's rights, so you're welcome to do merges yourself, once a PR is approved.

@kittaakos
Copy link
Contributor Author

it looks like you still have committer's rights

No, I do not have it 😊 The Eclipse page is misleading. But anyway, thanks for checking it and chiming in for the PR.

@vince-fugnitto vince-fugnitto added this to the 1.26.0 milestone May 26, 2022
kittaakos pushed a commit to arduino/arduino-ide that referenced this pull request Jul 25, 2022
kittaakos pushed a commit to kittaakos/arduino-ide that referenced this pull request Nov 8, 2022
kittaakos pushed a commit to kittaakos/arduino-ide that referenced this pull request Nov 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
preferences issues related to preferences
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants