From 316506c738a442064c66e7bd3e6c35f827986cb7 Mon Sep 17 00:00:00 2001 From: Nicolas Schweitzer Date: Tue, 4 Feb 2025 16:15:41 +0100 Subject: [PATCH] codereview: rename, remove useless comments, improve prints --- tasks/libs/common/check_tools_version.py | 8 ++++---- tasks/protobuf.py | 7 +++---- tasks/unit_tests/libs/common/check_tools_tests.py | 11 +++++------ 3 files changed, 12 insertions(+), 14 deletions(-) diff --git a/tasks/libs/common/check_tools_version.py b/tasks/libs/common/check_tools_version.py index 63ab655eef436c..fbab35f25ccdad 100644 --- a/tasks/libs/common/check_tools_version.py +++ b/tasks/libs/common/check_tools_version.py @@ -109,8 +109,8 @@ def check_tools_installed(tools: list) -> bool: """ Check if the tools are installed in the system. """ - for tool in tools: - if not shutil.which(tool): - print(f"{tool} is not installed. Please install it before running this task.") - return False + not_installed = [tool for tool in tools if not shutil.which(tool)] + if not_installed: + print(f"{color_message('ERROR', Color.RED)}: The following tools are not installed: {', '.join(not_installed)}") + return False return True diff --git a/tasks/protobuf.py b/tasks/protobuf.py index 93a693a0311157..bf1ca6d3217283 100644 --- a/tasks/protobuf.py +++ b/tasks/protobuf.py @@ -43,7 +43,7 @@ def generate(ctx, do_mockgen=True): We must build the packages one at a time due to protoc-gen-go limitations """ # Key: path, Value: grpc_gateway, inject_tags - check_dependencies(ctx) + check_tools(ctx) base = os.path.dirname(os.path.abspath(__file__)) repo_root = os.path.abspath(os.path.join(base, "..")) proto_root = os.path.join(repo_root, "pkg", "proto") @@ -151,7 +151,6 @@ def generate(ctx, do_mockgen=True): # Check the generated files were properly committed git_status = ctx.run("git status -suno", hide=True).stdout - # git_status = ctx.run("git status -suno | grep -E 'pkg/proto/pbgo.*.pb(.gw)?.go'").stdout.strip() proto_file = re.compile(r"pkg/proto/pbgo/.*\.pb(\.gw)?\.go$") if any(proto_file.search(line) for line in git_status.split("\n")): raise Exit( @@ -160,7 +159,7 @@ def generate(ctx, do_mockgen=True): ) -def check_dependencies(ctx): +def check_tools(ctx): """ Check if all the required dependencies are installed """ @@ -175,6 +174,6 @@ def check_dependencies(ctx): raise Exit( f"Expected protoc version {expected_version}, found {current_version}. Please run `inv install-protoc` before running this task.", code=1, - ) from None + ) except UnexpectedExit as e: raise Exit("protoc is not installed. Please install it before running this task.", code=1) from e diff --git a/tasks/unit_tests/libs/common/check_tools_tests.py b/tasks/unit_tests/libs/common/check_tools_tests.py index a4e0137ed06330..f390751c2f75c3 100644 --- a/tasks/unit_tests/libs/common/check_tools_tests.py +++ b/tasks/unit_tests/libs/common/check_tools_tests.py @@ -4,7 +4,7 @@ from invoke import Context, Exit, MockContext, Result, UnexpectedExit from tasks.libs.common.check_tools_version import check_tools_installed -from tasks.protobuf import check_dependencies +from tasks.protobuf import check_tools class TestCheckToolsInstalled(unittest.TestCase): @@ -18,13 +18,12 @@ def test_tool_not_installed(self): self.assertFalse(check_tools_installed(["not_installed_tool", "ls"])) -class TestCheckDependencies(unittest.TestCase): +class TestCheckTools(unittest.TestCase): @patch('tasks.protobuf.check_tools_installed', new=MagicMock(return_value=False)) def test_tools_not_installed(self): c = Context() with self.assertRaises(Exit) as e: - check_dependencies(c) - print(e) + check_tools(c) self.assertEqual( e.exception.message, "Please install the required tools with `inv install-tools` before running this task." ) @@ -33,7 +32,7 @@ def test_tools_not_installed(self): def test_bad_protoc(self): c = MockContext(run={'protoc --version': Result("libprotoc 1.98.2")}) with self.assertRaises(Exit) as e: - check_dependencies(c) + check_tools(c) self.assertTrue(e.exception.message.startswith("Expected protoc version 29.3, found")) @patch('tasks.protobuf.check_tools_installed', new=MagicMock(return_value=True)) @@ -41,5 +40,5 @@ def test_protoc_not_installed(self): c = MagicMock() c.run.side_effect = UnexpectedExit("protoc --version") with self.assertRaises(Exit) as e: - check_dependencies(c) + check_tools(c) self.assertEqual(e.exception.message, "protoc is not installed. Please install it before running this task.")