-
Notifications
You must be signed in to change notification settings - Fork 53
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
refactor: Clio Config #1593
refactor: Clio Config #1593
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1593 +/- ##
===========================================
+ Coverage 69.61% 70.06% +0.45%
===========================================
Files 257 265 +8
Lines 9879 10175 +296
Branches 5457 5612 +155
===========================================
+ Hits 6877 7129 +252
- Misses 1588 1601 +13
- Partials 1414 1445 +31 ☔ View full report in Codecov by Sentry. |
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.
Maybe we can just use one general constraint to constraint the port range, api range, instead of creating different constraint for each usage. For logger channels and levels, we can use one constraint (like OneOf) . I saw the similarity between the config constraint and the input validators, worth to check if anything we can borrow from validators.
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.
Leaving a few nits and findings. Good progress 👍
@godexsoft Just a heads-up: I removed the uint64 constraint since, in the places where it's used, the user only inputs a number in the thousands, and we multiply that number by a constant. Therefore, I really don't think the uint64 constraint is necessary |
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.
Looks good! just a few more tiny things to verify 👍
Add constraint + parse json into Config Second part of refactoring Clio Config; First PR found [here](#1544) Steps that are left to implement: - Replacing all the places where we fetch config values (by using config.valueOr/MaybeValue) to instead get it from Config Definition - Generate markdown file using Clio Config Description
Add constraint + parse json into Config
Second part of refactoring Clio Config; First PR found here
Steps that are left to implement: