-
Notifications
You must be signed in to change notification settings - Fork 388
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
[#6326] improve(CLI): Make CLI more extendable and maintainable. #6327
base: main
Are you sure you want to change the base?
Conversation
@justinmclean @tengqm , could you please review this PR when you have time? I’d really appreciate your feedback. plz review my new design |
I'll refrain from leaving comments here till I get a whole picture about where we are heading. |
Make CLI more extendable and maintainable.
77c9cbe
to
ced3ab8
Compare
Hi @Abyss-lord, thanks for the PR, while I feel this change is a little bit heavy, which may make the CLI code harder to maintain. Regarding the feature, would you add more information in the issue description, so that we can understand the scenario? Thank you! |
@shaofengshi As the CLI continues to evolve, we face growing challenges with code maintainability. There are two main issues we need to address:
To address these issues, I propose the following solutions: When the refactoring is complete the command just needs to pass the configuration + FullName, e.g. For the second problem, I've redesigned the OutputFormat interface to improve extensibility. I refactor the Now all of the CLI's information can be displayed using a single output method. Although this is a lot of code changes, IMHO I think it will be a big benefit for future client development. design doc: https://docs.google.com/document/d/1JU20XPHBMb-boOhdNmeZC9acyYupuj7EvQKfc1JaJBo/edit?usp=sharing |
What changes were proposed in this pull request?
Make CLI more extendable and maintainable.
design doc: https://docs.google.com/document/d/1JU20XPHBMb-boOhdNmeZC9acyYupuj7EvQKfc1JaJBo/edit?usp=sharing
Why are the changes needed?
Fix: #6326
Does this PR introduce any user-facing change?
No
How was this patch tested?
local test + ut