geruh commented on code in PR #3206:
URL: https://github.com/apache/iceberg-python/pull/3206#discussion_r3012842612
##########
tests/cli/test_console.py:
##########
@@ -62,17 +62,41 @@ def
test_hive_catalog_missing_uri_shows_helpful_error(mocker: MockFixture) -> No
assert "'uri'" not in result.output
[email protected](
+ "ignore:Deprecated in 0.11.0, will be removed in 1.0.0. "
+ "Please use `pyiceberg --version` instead of `pyiceberg
version`:DeprecationWarning"
+)
def test_version_does_not_load_catalog(mocker: MockFixture) -> None:
mock_load_catalog = mocker.patch("pyiceberg.cli.console.load_catalog",
side_effect=Exception("should not be called"))
runner = CliRunner()
result = runner.invoke(run, ["version"])
assert result.exit_code == 0
- assert result.output == f"{__version__}\n"
+ assert __version__ in result.output
mock_load_catalog.assert_not_called()
+def test_version_flag() -> None:
+ runner = CliRunner()
+ result = runner.invoke(run, ["--version"])
+
+ assert result.exit_code == 0
+ assert result.output == f"{__version__}\n"
+
+
+def test_version_command_emits_deprecation_warning(mocker: MockFixture) ->
None:
+ mocker.patch("pyiceberg.cli.console.load_catalog")
Review Comment:
Do we still need to mock the `load_catalog` with the previous change you
made in #3146?
##########
pyiceberg/cli/console.py:
##########
@@ -235,7 +237,17 @@ def location(ctx: Context, identifier: str) -> None:
@click.pass_context
@catch_exception()
def version(ctx: Context) -> None:
- """Print pyiceberg version."""
+ """Print pyiceberg's installed package number (deprecated, use
``--version`` instead)."""
Review Comment:
Feels like there's a lot of deprecation here for the CLI command.
The docstring change is good since it shows with `--help` but let's drop the
backticks since they don't render in help and keep the original wording. We
probably don't need deprecation_message function either since users interact
via CLI and the warning would be invisible to them by default.
##########
pyiceberg/cli/console.py:
##########
@@ -235,7 +237,17 @@ def location(ctx: Context, identifier: str) -> None:
@click.pass_context
@catch_exception()
def version(ctx: Context) -> None:
- """Print pyiceberg version."""
+ """Print pyiceberg's installed package number (deprecated, use
``--version`` instead)."""
+ deprecation_message(
+ deprecated_in="0.11.0",
+ removed_in="1.0.0",
+ help_message="Please use `pyiceberg --version` instead of `pyiceberg
version`",
+ )
+ click.echo(
+ "Deprecation warning: the `version` command is deprecated and will be
removed in 1.0.0. "
Review Comment:
+1 to this!
##########
tests/cli/test_console.py:
##########
@@ -62,17 +62,41 @@ def
test_hive_catalog_missing_uri_shows_helpful_error(mocker: MockFixture) -> No
assert "'uri'" not in result.output
[email protected](
+ "ignore:Deprecated in 0.11.0, will be removed in 1.0.0. "
+ "Please use `pyiceberg --version` instead of `pyiceberg
version`:DeprecationWarning"
+)
def test_version_does_not_load_catalog(mocker: MockFixture) -> None:
mock_load_catalog = mocker.patch("pyiceberg.cli.console.load_catalog",
side_effect=Exception("should not be called"))
runner = CliRunner()
result = runner.invoke(run, ["version"])
assert result.exit_code == 0
- assert result.output == f"{__version__}\n"
+ assert __version__ in result.output
Review Comment:
Nit: `result.stdout` gives stdout without the err mixed in. Do we still
need 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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
