-
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 |
It's because of the changes to vec anyway. It seems like tree shake transformer removes some types since it's generated wrongly like on the screen (comment TreeShake transform to reproduce): That's another reason why this kind of migration might be not suitable - there is a lot of small changes around the codebase that could broke something and I don't think the tests could cover all the small details of months of work to be sure it's done right. Also pr review for this is complicated. And there are definitely no benefits in doing this change right now. So I still suggest to revisit the approach and think about the points in the above discussion. |
Issue Reference(s):
Partially Fixes #3164