-
Notifications
You must be signed in to change notification settings - Fork 270
Fix CLI command order and catalog not found error #1828
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
base: main
Are you sure you want to change the base?
Conversation
pyiceberg/cli/console.py
Outdated
ctx.obj["catalog"] = load_catalog(ctx.obj["first_level_catalog"],**ctx.obj["properties"]) | ||
else: | ||
ctx.obj["catalog"] = load_catalog(ctx.obj["second_level_catalog"],**ctx.obj["properties"]) | ||
print("cataloglll",ctx.obj["catalog"]) |
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.
Nit: The forgot removed print
print("cataloglll",ctx.obj["catalog"]) |
pyiceberg/cli/console.py
Outdated
if ctx.obj["first_level_catalog"] is None and ctx.obj["second_level_catalog"] is None: | ||
ctx.obj["catalog"] = load_catalog(None, **ctx.obj["properties"]) | ||
elif ctx.obj["first_level_catalog"] is not None and ctx.obj["second_level_catalog"] is None: | ||
ctx.obj["catalog"] = load_catalog(ctx.obj["first_level_catalog"],**ctx.obj["properties"]) | ||
else: | ||
ctx.obj["catalog"] = load_catalog(ctx.obj["second_level_catalog"],**ctx.obj["properties"]) |
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.
We can refactor as:
if ctx.obj["first_level_catalog"] is None and ctx.obj["second_level_catalog"] is None: | |
ctx.obj["catalog"] = load_catalog(None, **ctx.obj["properties"]) | |
elif ctx.obj["first_level_catalog"] is not None and ctx.obj["second_level_catalog"] is None: | |
ctx.obj["catalog"] = load_catalog(ctx.obj["first_level_catalog"],**ctx.obj["properties"]) | |
else: | |
ctx.obj["catalog"] = load_catalog(ctx.obj["second_level_catalog"],**ctx.obj["properties"]) | |
catalog_option = ctx.obj.get("first_level_catalog",None) or ctx.obj.get("second_level_catalog",None) | |
ctx.obj["catalog"] = load_catalog(catalog_option,**ctx.obj["properties"]) |
pyiceberg/cli/console.py
Outdated
if not isinstance(ctx.obj["catalog"], Catalog): | ||
ctx.obj["output"].exception( | ||
ValueError("Could not determine catalog type from uri. REST (http/https) and Hive (thrift) is supported") | ||
ValueError("Could not determine catalog type from uri. REST (http/https) and Hive (thrift) is supported") |
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.
Nit: The indentation, as well as the order of imported modules, can be fixed based on the last comment.
ValueError("Could not determine catalog type from uri. REST (http/https) and Hive (thrift) is supported") | |
ValueError("Could not determine catalog type from uri. REST (http/https) and Hive (thrift) is supported") |
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 overall looks good, thanks !
Most of the nit can be resolved by autoformatting and linting via https://py.iceberg.apache.org/contributing/#linting.
make lint
Or install the pre-commit
can help run these formatting on commit.
Closes #1784
Rationale for this change
The command order for
pyiceberg list --catalog hive
would raise error; it should bepyiceberg --catalog hive list
.Now,
pyiceberg list --catalog hive
will return the same result aspyiceberg --catalog hive list
.Additionally, for cases where the catalog is not found:
For example, if the default catalog is not set and the command
pyiceberg list
is executed, it will raise an error stating:Default catalog not found. Please provide a catalog type using --type.
Are these changes tested?
The tests
test_no_catalog_error
andtest_list_with_catalog_option_wrong_order
intests/cli/test_console.py
cover these scenarios.Additionally, I fixed
test_missing_uri
. When a catalog exists but the URI is missing, it will now return an appropriate error.