-
Notifications
You must be signed in to change notification settings - Fork 531
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
Registry based config - Part 1 #975
Conversation
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.
while we're at it, we should add trailing comma lint @b-chu ?
Mostly looks good to me. not doing detailed pass yet, but I like the design and no major comments. some minor questions mostly for me
What's the appropriate level for testing here? |
@mvpatel2000 I did manual test of all the registration methods (see description), and have unit tests for all the registering methods. What other testing are you thinking? I could run the regression tests against this PR, not sure how much that adds though. |
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 but would like more eyes on this given big PR.
Do we need to update YAMLs?
@mvpatel2000 should not require any yaml updates as the yaml API is not changing. |
Adds the core of the registry system
Along for the ride, adds some __all__s. Will do a follow up PR that goes through and makes sure all the __all__s are correct.
registry-4-ia776l
, temporarily commented out some loggers and registered them through the different registry methods)