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

@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:

• this kind of migration will consist of multiple prs in similar manner where every pr does changes to the whole codebase
• such prs also had to update json/yaml fixtures every time that expected to be dropped eventually anyway
• that's literally sequence of breaking changes in the API from json/yaml on every pr to eventually drop the json/yaml support
• in order to migrate to ServiceDocument or other representation you need one big pr once again

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.

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.
I started to work on json/yaml removal

@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

@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

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 from_json are failing

haven't checked for N+1 but for from_json I can't traceback to the root cause

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 from_json are failing

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.

Action required: PR inactive for 5 days.
Status update or closure in 10 days.

PR closed after 10 days of inactivity.