justinmclean commented on code in PR #5836:
URL: https://github.com/apache/gravitino/pull/5836#discussion_r1879661056
##########
clients/cli/src/main/java/org/apache/gravitino/cli/commands/ListTables.java:
##########
@@ -64,6 +69,16 @@ public void handle() {
String all = Joiner.on(System.lineSeparator()).join(tableNames);
- System.out.println(all.toString());
+ System.out.println(all);
+ }
+
+ private void showUnSpecifiedMessage() {
+ if (metalake == null) {
+ System.err.println(ErrorMessages.UNSPECIFIED_METALAKE);
+ } else if (catalog == null) {
+ System.err.println(ErrorMessages.UNSPECIFIED_CATALOG);
+ } else {
+ System.err.println(ErrorMessages.UNSPECIFIED_SCHEMA);
+ }
Review Comment:
Perhaps make this return a boolean and use that to exit early? Just a
suggestion, but I think that may work better. What do you think?
##########
clients/cli/src/main/java/org/apache/gravitino/cli/commands/ListTables.java:
##########
@@ -48,6 +49,10 @@ public ListTables(
/** List the names of all tables in a schema. */
@Override
public void handle() {
+ if (metalake == null || catalog == null || schema == null) {
+ showUnSpecifiedMessage();
+ return;
+ }
Review Comment:
Yes you can modify it, but I don't think all subclasses would need to change.
##########
clients/cli/src/main/java/org/apache/gravitino/cli/commands/CommandConstants.java:
##########
Review Comment:
Also the list full list commands doesn't match the list of metadata types.
##########
clients/cli/src/main/java/org/apache/gravitino/cli/commands/CommandConstants.java:
##########
Review Comment:
Ah sorry I misunderstood you, but there is no need for `CommandConstants` at
all, as these are already defined in `CommandEntities`. Note that
`CommandEntities` might not match `MetadataObject Type`, but there is overlap.
##########
clients/cli/src/main/java/org/apache/gravitino/cli/commands/CommandConstants.java:
##########
Review Comment:
That's probably not going to help, as we're dealing with actual entity names
here, and it seems unrelated to this PR?
##########
clients/cli/src/main/java/org/apache/gravitino/cli/GravitinoCommandLine.java:
##########
@@ -316,6 +321,18 @@ private void handleTableCommand() {
Command.setAuthenticationMode(auth, userName);
if (CommandActions.LIST.equals(command)) {
+ List<String> missArguments =
Review Comment:
This name could be improved - there are not really arguments but part of the
name argument (except metalake). Perhaps missingNames or missingEntities?
##########
clients/cli/src/main/java/org/apache/gravitino/cli/GravitinoCommandLine.java:
##########
@@ -316,6 +321,18 @@ private void handleTableCommand() {
Command.setAuthenticationMode(auth, userName);
if (CommandActions.LIST.equals(command)) {
+ List<String> missArguments =
+ Stream.of(
+ metalake == null ? MetadataObject.Type.METALAKE.name() :
null,
+ catalog == null ? MetadataObject.Type.CATALOG.name() : null,
+ schema == null ? MetadataObject.Type.SCHEMA.name() : null)
Review Comment:
I would not use `MetadataObject.Type.METALAKE` etc. use the constants
defined in `CommandEntities.java`
##########
clients/cli/src/main/java/org/apache/gravitino/cli/commands/ListTables.java:
##########
@@ -48,6 +49,10 @@ public ListTables(
/** List the names of all tables in a schema. */
@Override
public void handle() {
+ if (metalake == null || catalog == null || schema == null) {
+ showUnSpecifiedMessage();
+ return;
+ }
Review Comment:
Yes I don't think you need to check for that as you'll get "Unknown metalake
name" if you don't specify it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
