Skip to content
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

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

ssddOnTop
Copy link
Member

Issue Reference(s):
Partially Fixes #3164

@github-actions github-actions bot added the type: chore Routine tasks like conversions, reorganization, and maintenance work. label Dec 4, 2024
@meskill
Copy link
Contributor

meskill commented Dec 4, 2024

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?

@ssddOnTop
Copy link
Member Author

@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
Copy link
Contributor

meskill commented Dec 4, 2024

@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

@tusharmath
Copy link
Contributor

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.

@meskill
Copy link
Contributor

meskill commented Dec 5, 2024

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

@ssddOnTop
Copy link
Member Author

@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
Copy link
Contributor

meskill commented Dec 5, 2024

@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

@ssddOnTop
Copy link
Member Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: chore Routine tasks like conversions, reorganization, and maintenance work.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[master] redesign configuration Interface
3 participants