-
Notifications
You must be signed in to change notification settings - Fork 65
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
Adds JSON Schema for Fixit configs. #188
Conversation
Codecov Report
@@ Coverage Diff @@
## master #188 +/- ##
==========================================
+ Coverage 85.93% 86.03% +0.09%
==========================================
Files 90 91 +1
Lines 3691 3702 +11
==========================================
+ Hits 3172 3185 +13
+ Misses 519 517 -2
Continue to review full report at Codecov.
|
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's great that the config validation is simpler now. The changes is not ready and has the following errors.
There is an error on loading the schema file in CI builds: https://github.com/Instagram/Fixit/pull/188/checks?check_run_id=2453268372
Type error: https://github.com/Instagram/Fixit/runs/2453268393
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.
LGTM. Found one thing can be improved in the test case.
Type errors are unrelated to this diff, I will create a new PR to fix those. |
Pyre PR: #196 |
Summary
In the long run adding a JSON Schema will give us the ability to configure which keys of inheritable configs should be overridden vs merged. See my full explanation on #183, happy to discuss the long term plan there as well.
Adding this today is a step towards inheritable configs, as well as it removes the need for the code doing type checking on the configs. We can configure it to be more specific if we want as well (like what keys are required vs optional, that sort of thing).
Test Plan
Added some unit tests.