From 93381a7343ab867876d2b5b4eb2e20dec4844197 Mon Sep 17 00:00:00 2001 From: Pere Miquel Brull Date: Thu, 17 Nov 2022 17:44:26 +0100 Subject: [PATCH] Add Pylint print checker and py-checkstyle as required (#8849) * Always run the checkstyle * Add print checker lint plugin * Typos * Add print checker * Fix e2e --- .github/workflows/py-checkstyle.yml | 3 -- .pylintrc | 1 + Makefile | 4 +- ingestion/plugins/README.md | 6 +++ ingestion/plugins/__init__.py | 0 ingestion/plugins/print_checker.py | 53 +++++++++++++++++++ ingestion/src/metadata/utils/ansi.py | 2 +- ingestion/src/metadata/utils/time_utils.py | 15 +++--- ingestion/tests/cli_e2e/test_cli_mysql.py | 2 +- .../kpi/test_registry_functions.py | 2 - ingestion/tests/unit/source/test_sagemaker.py | 1 - 11 files changed, 71 insertions(+), 18 deletions(-) create mode 100644 ingestion/plugins/README.md create mode 100644 ingestion/plugins/__init__.py create mode 100644 ingestion/plugins/print_checker.py diff --git a/.github/workflows/py-checkstyle.yml b/.github/workflows/py-checkstyle.yml index ffa6d856fa4..a127f615bbf 100644 --- a/.github/workflows/py-checkstyle.yml +++ b/.github/workflows/py-checkstyle.yml @@ -16,9 +16,6 @@ name: Python Checkstyle on: pull_request_target: types: [labeled, opened, synchronize, reopened] - branches: - - main - - '0.[0-9]+.[0-9]+' permissions: contents: read diff --git a/.pylintrc b/.pylintrc index 26fff97affb..bf30dcff3c5 100644 --- a/.pylintrc +++ b/.pylintrc @@ -19,6 +19,7 @@ good-names=T,C,fn,db fail-under=6.0 init-hook='from pylint.config import find_pylintrc; import os, sys; sys.path.append(os.path.dirname(find_pylintrc()))' extension-pkg-allow-list=pydantic +load-plugins=ingestion.plugins.print_checker [MESSAGES CONTROL] disable=no-name-in-module,import-error,duplicate-code diff --git a/Makefile b/Makefile index 2745b2ee4ee..95954ebd89b 100644 --- a/Makefile +++ b/Makefile @@ -41,7 +41,7 @@ precommit_install: ## Install the project's precommit hooks from .pre-commit-co .PHONY: lint lint: ## Run pylint on the Python sources to analyze the codebase - find $(PY_SOURCE) -path $(PY_SOURCE)/metadata/generated -prune -false -o -type f -name "*.py" | xargs pylint --ignore-paths=$(PY_SOURCE)/metadata_server/ + PYTHONPATH="${PYTHONPATH}:./ingestion/plugins" find $(PY_SOURCE) -path $(PY_SOURCE)/metadata/generated -prune -false -o -type f -name "*.py" | xargs pylint --ignore-paths=$(PY_SOURCE)/metadata_server/ .PHONY: py_format py_format: ## Run black and isort to format the Python codebase @@ -54,7 +54,7 @@ py_format_check: ## Check if Python sources are correctly formatted pycln ingestion/ openmetadata-airflow-apis/ --diff --extend-exclude $(PY_SOURCE)/metadata/generated isort --check-only ingestion/ openmetadata-airflow-apis/ --skip $(PY_SOURCE)/metadata/generated --skip ingestion/build --profile black --multi-line 3 black --check --diff ingestion/ openmetadata-airflow-apis/ --extend-exclude $(PY_SOURCE)/metadata/generated - pylint --fail-under=10 $(PY_SOURCE)/metadata --ignore-paths $(PY_SOURCE)/metadata/generated || (echo "PyLint error code $$?"; exit 1) + PYTHONPATH="${PYTHONPATH}:./ingestion/plugins" pylint --fail-under=10 $(PY_SOURCE)/metadata --ignore-paths $(PY_SOURCE)/metadata/generated || (echo "PyLint error code $$?"; exit 1) ## Ingestion models generation .PHONY: generate diff --git a/ingestion/plugins/README.md b/ingestion/plugins/README.md new file mode 100644 index 00000000000..bbb1564f4c4 --- /dev/null +++ b/ingestion/plugins/README.md @@ -0,0 +1,6 @@ +# Custom PyLint plugins + +- `pint_checker`: to handle `print` statements as warnings. + - Add it to `.pylintrc` as `load-plugins=ingestion.plugins.print_checker` under `[MASTER]`. + +You'll need to update the path of `pylint` at runtime with `PYTHONPATH="${PYTHONPATH}:./ingestion/plugins"`. diff --git a/ingestion/plugins/__init__.py b/ingestion/plugins/__init__.py new file mode 100644 index 00000000000..e69de29bb2d diff --git a/ingestion/plugins/print_checker.py b/ingestion/plugins/print_checker.py new file mode 100644 index 00000000000..2f1d43e24c6 --- /dev/null +++ b/ingestion/plugins/print_checker.py @@ -0,0 +1,53 @@ +# Copyright 2022 Collate +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# http://www.apache.org/licenses/LICENSE-2.0 +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +""" +Custom pylint plugin to catch `print` calls +""" +from typing import TYPE_CHECKING + +from astroid import nodes +from pylint.checkers import BaseChecker +from pylint.checkers.utils import only_required_for_messages +from pylint.interfaces import IAstroidChecker + +if TYPE_CHECKING: + from pylint.lint import PyLinter + + +class PrintChecker(BaseChecker): + """ + Check for any print statement in the code + """ + + __implements__ = (IAstroidChecker,) + name = "no_print_allowed" + _symbol = "print-call" + msgs = { + "W5001": ( + "Used builtin function %s", + _symbol, + "Print can make us lose traceability, use logging instead", + ) + } + + @only_required_for_messages("print-call") + def visit_call(self, node: nodes.Call) -> None: + if isinstance(node.func, nodes.Name) and node.func.name == "print": + self.add_message(self._symbol, node=node) + + +def register(linter: "PyLinter") -> None: + """ + This required method auto registers the checker during initialization. + :param linter: The linter to register the checker to. + """ + linter.register_checker(PrintChecker(linter)) diff --git a/ingestion/src/metadata/utils/ansi.py b/ingestion/src/metadata/utils/ansi.py index 8795df803d3..76d827ebfc9 100644 --- a/ingestion/src/metadata/utils/ansi.py +++ b/ingestion/src/metadata/utils/ansi.py @@ -31,6 +31,6 @@ class ANSI(Enum): def print_ansi_encoded_string( color: Optional[ANSI] = None, bold: bool = False, message: str = "" ): - print( + print( # pylint: disable=W5001 f"{ANSI.BOLD.value if bold else ''}{color.value if color else ''}{message}{ANSI.ENDC.value}" ) diff --git a/ingestion/src/metadata/utils/time_utils.py b/ingestion/src/metadata/utils/time_utils.py index ccafb3fa2dc..3ffbcfb6a01 100644 --- a/ingestion/src/metadata/utils/time_utils.py +++ b/ingestion/src/metadata/utils/time_utils.py @@ -41,19 +41,18 @@ def get_beginning_of_day_timestamp_mill( """Get the beginning of day timestamp Args: - days (int, optional): delat in days. Defaults to 0. - seconds (int, optional): delat in seconds. Defaults to 0. - microseconds (int, optional): delat in microseconds. Defaults to 0. - milliseconds (int, optional): delat in milliseconds. Defaults to 0. - minutes (int, optional): delat in minutes. Defaults to 0. - hours (int, optional): delat in hours. Defaults to 0. - weeks (int, optional): delat in weeks. Defaults to 0. + days (int, optional): delay in days. Defaults to 0. + seconds (int, optional): delay in seconds. Defaults to 0. + microseconds (int, optional): delay in microseconds. Defaults to 0. + milliseconds (int, optional): delay in milliseconds. Defaults to 0. + minutes (int, optional): delay in minutes. Defaults to 0. + hours (int, optional): delay in hours. Defaults to 0. + weeks (int, optional): delay in weeks. Defaults to 0. Returns: int: timestamp milliseconds """ now_utc = datetime.utcnow() - print(now_utc) delta = timedelta( weeks=weeks, days=days, diff --git a/ingestion/tests/cli_e2e/test_cli_mysql.py b/ingestion/tests/cli_e2e/test_cli_mysql.py index fae2aef631b..075dd8aeea5 100644 --- a/ingestion/tests/cli_e2e/test_cli_mysql.py +++ b/ingestion/tests/cli_e2e/test_cli_mysql.py @@ -96,7 +96,7 @@ class MysqlCliTest(CliCommonDB.TestSuite): @staticmethod def expected_filtered_table_includes() -> int: - return 43 + return 44 @staticmethod def expected_filtered_table_excludes() -> int: diff --git a/ingestion/tests/unit/data_insight/kpi/test_registry_functions.py b/ingestion/tests/unit/data_insight/kpi/test_registry_functions.py index 121bf62e8b3..dc51666873b 100644 --- a/ingestion/tests/unit/data_insight/kpi/test_registry_functions.py +++ b/ingestion/tests/unit/data_insight/kpi/test_registry_functions.py @@ -205,8 +205,6 @@ def test_percentage_of_entities_with_owner_kpi_result(): kpi_target, results, "completedOwner", 1668083253659 ) - print(kpi_result) - assert kpi_result.targetResult for result in kpi_result.targetResult: if result.name == "hasOwnerFraction": diff --git a/ingestion/tests/unit/source/test_sagemaker.py b/ingestion/tests/unit/source/test_sagemaker.py index dd355e4e143..e70bea4c65c 100644 --- a/ingestion/tests/unit/source/test_sagemaker.py +++ b/ingestion/tests/unit/source/test_sagemaker.py @@ -73,7 +73,6 @@ def _setup_mock_sagemaker(create_model: bool = False): sagemaker = boto3.Session().client("sagemaker") if not create_model: return - print("Creating model!!!!!!!") sagemaker.create_model( ModelName="mock-model", PrimaryContainer={