-
Notifications
You must be signed in to change notification settings - Fork 256
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
chore: make config.types
to Vec<Type>
#3195
base: main
Are you sure you want to change the base?
Conversation
Could you please explain what is high level goal in making such refactoring here? and how it relates to the another pr #3170 I think one of the goals of #3164 is to minimize or drop config entirely since and focus on ServiceDocument. In such a case is there any better option to implement the migration? Can we have a trait/s that could be implemented both by Config and ServiceDocument and gradually migrate to the abstraction? |
@meskill we can have traits but it will just make stuff less maintainable @tusharmath is thinking thinking to gradually convert structure config to that of service doc, later we can either drop config, or will have ability to throw better errors from config directly |
Could you elaborate what do you mean by "traits making stuff less maintainable"? With current approach I see the following issues:
That's why I'm suggesting to consider different approaches like traits and/or intermediate types that could help in the migration process |
We can first drop json yaml support from our CLI and update docs that should introduce one breaking change. The code then can continue getting refactored. Json and Yaml aren't that used as much. The whole codebase changes are another thing. I would solve that by adding a function that returns types and make the field private. Slowly do this for enums and unions also. Effectively turn config to actually internally embed a service doc directly. |
That makes more sense. |
@meskill can you help with the fialing tests? I guess just these 3 tests are failing.. I can't find the root cause for that |
I see more than 3 tests failing and most of them I think failing because the change from BTreeMap to Vec changed the order of types in some cases |
it's not because of the order, one N+1 test is failing and all haven't checked for N+1 but for from_json I can't traceback to the root cause |
Issue Reference(s):
Partially Fixes #3164