This is an automated email from the ASF dual-hosted git repository. ephraimanierobi pushed a commit to branch sync_v2_10_test in repository https://gitbox.apache.org/repos/asf/airflow.git
commit 9169d64acc068f5f7f83a9d13979748f139e65a6 Author: Wei Lee <[email protected]> AuthorDate: Sun Sep 8 18:08:41 2024 +0800 Autofix default deferrable with LibCST (#42089) * ci: auto fix default_deferrable value with LibCST * ci(pre-commit): lower minimum libcst version to 1.1.0 for python 3.8 support (#42083) --- .pre-commit-config.yaml | 13 +-- contributing-docs/08_static_code_checks.rst | 2 +- dev/breeze/doc/images/output_static-checks.svg | 2 +- dev/breeze/doc/images/output_static-checks.txt | 2 +- dev/breeze/src/airflow_breeze/pre_commit_ids.py | 2 +- scripts/ci/pre_commit/check_deferrable_default.py | 111 ++++++++++++---------- 6 files changed, 73 insertions(+), 59 deletions(-) diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index b4b6ec9c3d..94918da9d1 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -218,6 +218,13 @@ repos: files: ^airflow/models/taskinstance.py$|^airflow/models/taskinstancehistory.py$ pass_filenames: false require_serial: true + - id: check-deferrable-default + name: Check and fix default value of default_deferrable + language: python + entry: ./scripts/ci/pre_commit/check_deferrable_default.py + pass_filenames: false + additional_dependencies: ["libcst>=1.1.0"] + files: ^airflow/.*/sensors/.*\.py$|^airflow/.*/operators/.*\.py$ - repo: https://github.com/asottile/blacken-docs rev: 1.18.0 hooks: @@ -1134,12 +1141,6 @@ repos: pass_filenames: true files: \.py$ exclude: ^airflow/providers|^dev/.*\.py$|^scripts/.*\.py$|^tests/|^\w+_tests/|^docs/.*\.py$|^airflow/utils/helpers.py$|^hatch_build.py$ - - id: check-deferrable-default-value - name: Check default value of deferrable attribute - language: python - entry: ./scripts/ci/pre_commit/check_deferrable_default.py - pass_filenames: false - files: ^airflow/.*/sensors/.*\.py$|^airflow/.*/operators/.*\.py$ - id: check-provider-docs-valid name: Validate provider doc files entry: ./scripts/ci/pre_commit/check_provider_docs.py diff --git a/contributing-docs/08_static_code_checks.rst b/contributing-docs/08_static_code_checks.rst index e1312b8a45..b122b34867 100644 --- a/contributing-docs/08_static_code_checks.rst +++ b/contributing-docs/08_static_code_checks.rst @@ -160,7 +160,7 @@ require Breeze Docker image to be built locally. +-----------------------------------------------------------+--------------------------------------------------------------+---------+ | check-decorated-operator-implements-custom-name | Check @task decorator implements custom_operator_name | | +-----------------------------------------------------------+--------------------------------------------------------------+---------+ -| check-deferrable-default-value | Check default value of deferrable attribute | | +| check-deferrable-default | Check and fix default value of default_deferrable | | +-----------------------------------------------------------+--------------------------------------------------------------+---------+ | check-docstring-param-types | Check that docstrings do not specify param types | | +-----------------------------------------------------------+--------------------------------------------------------------+---------+ diff --git a/dev/breeze/doc/images/output_static-checks.svg b/dev/breeze/doc/images/output_static-checks.svg index 12e58b0b2d..de796de4e0 100644 --- a/dev/breeze/doc/images/output_static-checks.svg +++ b/dev/breeze/doc/images/output_static-checks.svg @@ -332,7 +332,7 @@ </text><text class="breeze-static-checks-r5" x="0" y="361.6" textLength="12.2" clip-path="url(#breeze-static-checks-line-14)">│</text><text class="breeze-static-checks-r7" x="451.4" y="361.6" textLength="988.2" clip-path="url(#breeze-static-checks-line-14)">check-code-deprecations | check-common-compat-used-for-openlineage |             </text><text class="breeze-static-checks-r5" x="1451.8" y="361.6" textLen [...] </text><text class="breeze-static-checks-r5" x="0" y="386" textLength="12.2" clip-path="url(#breeze-static-checks-line-15)">│</text><text class="breeze-static-checks-r7" x="451.4" y="386" textLength="988.2" clip-path="url(#breeze-static-checks-line-15)">check-compat-cache-on-methods | check-core-deprecation-classes |                 </text><text class="breeze-static-checks-r5" x="1451.8" y [...] </text><text class="breeze-static-checks-r5" x="0" y="410.4" textLength="12.2" clip-path="url(#breeze-static-checks-line-16)">│</text><text class="breeze-static-checks-r7" x="451.4" y="410.4" textLength="988.2" clip-path="url(#breeze-static-checks-line-16)">check-daysago-import-from-utils | check-decorated-operator-implements-custom-name</text><text class="breeze-static-checks-r5" x="1451.8" y="410.4" textLength="12.2" clip-path="url(#breeze-static-checks-line-16)">│</text><tex [...] -</text><text class="breeze-static-checks-r5" x="0" y="434.8" textLength="12.2" clip-path="url(#breeze-static-checks-line-17)">│</text><text class="breeze-static-checks-r7" x="451.4" y="434.8" textLength="988.2" clip-path="url(#breeze-static-checks-line-17)">| check-deferrable-default-value | check-docstring-param-types |                 </text><text class="breeze-static-checks-r5" x=" [...] +</text><text class="breeze-static-checks-r5" x="0" y="434.8" textLength="12.2" clip-path="url(#breeze-static-checks-line-17)">│</text><text class="breeze-static-checks-r7" x="451.4" y="434.8" textLength="988.2" clip-path="url(#breeze-static-checks-line-17)">| check-deferrable-default | check-docstring-param-types |                       </text><text class [...] </text><text class="breeze-static-checks-r5" x="0" y="459.2" textLength="12.2" clip-path="url(#breeze-static-checks-line-18)">│</text><text class="breeze-static-checks-r7" x="451.4" y="459.2" textLength="988.2" clip-path="url(#breeze-static-checks-line-18)">check-example-dags-urls | check-executables-have-shebangs |                      </text><text class="breeze-s [...] </text><text class="breeze-static-checks-r5" x="0" y="483.6" textLength="12.2" clip-path="url(#breeze-static-checks-line-19)">│</text><text class="breeze-static-checks-r7" x="451.4" y="483.6" textLength="988.2" clip-path="url(#breeze-static-checks-line-19)">check-extra-packages-references | check-extras-order | check-fab-migrations |    </text><text class="breeze-static-checks-r5" x="1451.8" y="483.6" textLength="12.2" clip-path="url(#breeze-s [...] </text><text class="breeze-static-checks-r5" x="0" y="508" textLength="12.2" clip-path="url(#breeze-static-checks-line-20)">│</text><text class="breeze-static-checks-r7" x="451.4" y="508" textLength="988.2" clip-path="url(#breeze-static-checks-line-20)">check-for-inclusive-language | check-get-lineage-collector-providers |           </text><text class="breeze-static-checks-r5" x="1451.8" y="508" textLength="12.2" clip- [...] diff --git a/dev/breeze/doc/images/output_static-checks.txt b/dev/breeze/doc/images/output_static-checks.txt index 9453c31f4d..53bb9e5b46 100644 --- a/dev/breeze/doc/images/output_static-checks.txt +++ b/dev/breeze/doc/images/output_static-checks.txt @@ -1 +1 @@ -85fff776e1e23ae5f9715b38c1f71825 +97228aa2e9cdff69f86a9c0dfee5f83e diff --git a/dev/breeze/src/airflow_breeze/pre_commit_ids.py b/dev/breeze/src/airflow_breeze/pre_commit_ids.py index be599e1f7d..70200d1ec6 100644 --- a/dev/breeze/src/airflow_breeze/pre_commit_ids.py +++ b/dev/breeze/src/airflow_breeze/pre_commit_ids.py @@ -45,7 +45,7 @@ PRE_COMMIT_LIST = [ "check-core-deprecation-classes", "check-daysago-import-from-utils", "check-decorated-operator-implements-custom-name", - "check-deferrable-default-value", + "check-deferrable-default", "check-docstring-param-types", "check-example-dags-urls", "check-executables-have-shebangs", diff --git a/scripts/ci/pre_commit/check_deferrable_default.py b/scripts/ci/pre_commit/check_deferrable_default.py index 1e1fd9c7a6..bfde61f231 100755 --- a/scripts/ci/pre_commit/check_deferrable_default.py +++ b/scripts/ci/pre_commit/check_deferrable_default.py @@ -25,6 +25,10 @@ import os import sys from typing import Iterator +import libcst as cst +from libcst.codemod import CodemodContext +from libcst.codemod.visitors import AddImportsVisitor + ROOT_DIR = os.path.abspath(os.path.join(os.path.dirname(__file__), os.pardir, os.pardir, os.pardir)) DEFERRABLE_DOC = ( @@ -33,62 +37,71 @@ DEFERRABLE_DOC = ( ) +class DefaultDeferrableVisitor(ast.NodeVisitor): + def __init__(self, *args, **kwargs) -> None: + super().__init__(*args, *kwargs) + self.error_linenos: list[int] = [] + + def visit_FunctionDef(self, node: ast.FunctionDef) -> ast.FunctionDef: + if node.name == "__init__": + args = node.args + arguments = reversed([*args.args, *args.posonlyargs, *args.kwonlyargs]) + defaults = reversed([*args.defaults, *args.kw_defaults]) + for argument, default in itertools.zip_longest(arguments, defaults): + # argument is not deferrable + if argument is None or argument.arg != "deferrable": + continue + + # argument is deferrable, but comes with no default value + if default is None: + self.error_linenos.append(argument.lineno) + continue + + # argument is deferrable, but the default value is not valid + if not _is_valid_deferrable_default(default): + self.error_linenos.append(default.lineno) + return node + + +class DefaultDeferrableTransformer(cst.CSTTransformer): + def leave_Param(self, original_node: cst.Param, updated_node: cst.Param) -> cst.Param: + if original_node.name.value == "deferrable": + expected_default_cst = cst.parse_expression( + 'conf.getboolean("operators", "default_deferrable", fallback=False)' + ) + + if updated_node.default and updated_node.default.deep_equals(expected_default_cst): + return updated_node + return updated_node.with_changes(default=expected_default_cst) + return updated_node + + def _is_valid_deferrable_default(default: ast.AST) -> bool: """Check whether default is 'conf.getboolean("operators", "default_deferrable", fallback=False)'""" - if not isinstance(default, ast.Call): - return False # Not a function call. - - # Check the function callee is exactly 'conf.getboolean'. - call_to_conf_getboolean = ( - isinstance(default.func, ast.Attribute) - and isinstance(default.func.value, ast.Name) - and default.func.value.id == "conf" - and default.func.attr == "getboolean" - ) - if not call_to_conf_getboolean: - return False - - # Check arguments. - return ( - len(default.args) == 2 - and isinstance(default.args[0], ast.Constant) - and default.args[0].value == "operators" - and isinstance(default.args[1], ast.Constant) - and default.args[1].value == "default_deferrable" - and len(default.keywords) == 1 - and default.keywords[0].arg == "fallback" - and isinstance(default.keywords[0].value, ast.Constant) - and default.keywords[0].value.value is False - ) + return ast.unparse(default) == "conf.getboolean('operators', 'default_deferrable', fallback=False)" def iter_check_deferrable_default_errors(module_filename: str) -> Iterator[str]: - ast_obj = ast.parse(open(module_filename).read()) - cls_nodes = (node for node in ast.iter_child_nodes(ast_obj) if isinstance(node, ast.ClassDef)) - init_method_nodes = ( - node - for cls_node in cls_nodes - for node in ast.iter_child_nodes(cls_node) - if isinstance(node, ast.FunctionDef) and node.name == "__init__" - ) + ast_tree = ast.parse(open(module_filename).read()) + visitor = DefaultDeferrableVisitor() + visitor.visit(ast_tree) + # We check the module using the ast once and then fix it through cst if needed. + # The primary reason we don't do it all through cst is performance. + if visitor.error_linenos: + _fix_invalide_deferrable_default_value(module_filename) + yield from (f"{module_filename}:{lineno}" for lineno in visitor.error_linenos) - for node in init_method_nodes: - args = node.args - arguments = reversed([*args.args, *args.posonlyargs, *args.kwonlyargs]) - defaults = reversed([*args.defaults, *args.kw_defaults]) - for argument, default in itertools.zip_longest(arguments, defaults): - # argument is not deferrable - if argument is None or argument.arg != "deferrable": - continue - # argument is deferrable, but comes with no default value - if default is None: - yield f"{module_filename}:{argument.lineno}" - continue +def _fix_invalide_deferrable_default_value(module_filename: str) -> None: + context = CodemodContext(filename=module_filename) + AddImportsVisitor.add_needed_import(context, "airflow.configuration", "conf") + transformer = DefaultDeferrableTransformer() - # argument is deferrable, but the default value is not valid - if not _is_valid_deferrable_default(default): - yield f"{module_filename}:{default.lineno}" + source_cst_tree = cst.parse_module(open(module_filename).read()) + modified_cst_tree = AddImportsVisitor(context).transform_module(source_cst_tree.visit(transformer)) + if not source_cst_tree.deep_equals(modified_cst_tree): + with open(module_filename, "w") as writer: + writer.write(modified_cst_tree.code) def main() -> int: @@ -103,7 +116,7 @@ def main() -> int: for error in errors: print(f" {error}") print( - """Please set the default value of deferrbale to """ + """Please set the default value of deferrable to """ """"conf.getboolean("operators", "default_deferrable", fallback=False)"\n""" f"See: {DEFERRABLE_DOC}\n" )
