-
Notifications
You must be signed in to change notification settings - Fork 382
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
[#6139] Refactor metalake command in Gravitino CLI #6140
base: main
Are you sure you want to change the base?
Conversation
@shaofengshi and @tengqm what do you think of this? It's mostly cosmetic but does make it easier to understand. |
@justinmclean Should we split other commands? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this clean approach.
* Constructs a MetalakeCommandHandler instance. | ||
* | ||
* @param gravitinoCommandLine The Gravitino command line instance. | ||
* @param line The command line options. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rename line
to options
or arguments
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The variable name line
is used elsewhere e.g. FullName, GravitinoCommandLine and Main, options might be a little confusing as that is a different class.
@Abyss-lord You can start working on the other sub-issues for this if you want. |
@shaofengshi can you review this? |
What changes were proposed in this pull request?
The Gravitino command line class is a little large and could be broken up.
Why are the changes needed?
For readability and maintainability.
Fix: #6139
Does this PR introduce any user-facing change?
None.
How was this patch tested?
Tested locally.