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

[#5861] improvement(CLI): Refactor the validation logic in the handle methods #5972

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Abyss-lord
Copy link
Contributor

@Abyss-lord Abyss-lord commented Dec 24, 2024

What changes were proposed in this pull request?

refactor the validation logic of all entities and add test case, just like validation of table command #5906 . A hint is provided when the user's output is missing the required arguments. for example:

gcli column list -m demo_metalake, --name Hive_catalog
# Malformed entity name.
# Missing required argument(s): schema, table

gcli column details -m demo_metalake, --name Hive_catalog --audit
# Malformed entity name.
# Missing required argument(s): schema, table, column

gcli user delete -m demo_metalake
Missing --user option.

Currently, the Role command needs to be refactored and opened as a separate issue

Why are the changes needed?

Fix: #5861

Does this PR introduce any user-facing change?

NO

How was this patch tested?

local test

…handle methods

refactor the validation logic of all entities and add test case.
@Abyss-lord
Copy link
Contributor Author

Abyss-lord commented Dec 24, 2024

Hi @justinmclean @xunliu @tengqm @mchades , could you please review this PR when you have time? I’d really appreciate your feedback.

Copy link
Contributor

@tengqm tengqm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding the scope, this PR looks good to me other than a typo.
However, I would NOT structure the validation of options/args this way. I'd delegate the validation to those command objects. I.e.

  • Treat this class as a factory of command objects and focus
    on creating and invoking the correct commands.
  • Let the command objects do their own validation and execution.
  • To avoid a huge refactoring, we can treat parameter validation a part of the handle() workflow.

However, this belongs to a different (series of) PRs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Improvement] Refactor the validation logic in the handle methods
2 participants