This is an automated email from the ASF dual-hosted git repository.
potiuk pushed a commit to branch v2-10-test
in repository https://gitbox.apache.org/repos/asf/airflow.git
The following commit(s) were added to refs/heads/v2-10-test by this push:
new 0a69b8b172 Autofix default deferrable with LibCST (#42089)
0a69b8b172 is described below
commit 0a69b8b172895f1ebe0af1452e65db63fdf0e149
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"
)
