Copilot commented on code in PR #64812:
URL: https://github.com/apache/airflow/pull/64812#discussion_r3066502422
##########
airflow-ctl/src/airflowctl/ctl/cli_config.py:
##########
@@ -566,11 +579,15 @@ def _create_args_map_from_operation(self):
args = []
for parameter in operation.get("parameters"):
for parameter_key, parameter_type in parameter.items():
+ if parameter_key == "has_default":
+ continue
if self._is_primitive_type(type_name=parameter_type):
is_bool = parameter_type == "bool"
+ is_positional = not is_bool and not
parameter.get("has_default", False)
+ sanitized_key =
self._sanitize_arg_parameter_key(parameter_key)
args.append(
self._create_arg(
- arg_flags=("--" +
self._sanitize_arg_parameter_key(parameter_key),),
+ arg_flags=(parameter_key,) if is_positional
else ("--" + sanitized_key,),
Review Comment:
Related to the parameter-shape issue: the `for parameter_key, parameter_type
in parameter.items()` loop plus `if parameter_key == \"has_default\": continue`
is an avoidable control-flow hack that spreads across multiple call sites (also
in `_get_func`). If you keep the current shape, a safer alternative is to store
metadata under a dedicated nested key (e.g. `{\"name\": ..., \"type\": ...,
\"meta\": {\"has_default\": ...}}`) or move metadata out of the per-parameter
dict entirely so this loop can be simplified and cannot accidentally skip real
parameters.
##########
airflow-ctl/src/airflowctl/ctl/cli_config.py:
##########
@@ -410,11 +410,16 @@ def get_function_details(node: ast.FunctionDef,
parent_node: ast.ClassDef) -> di
args = []
return_annotation: str = ""
- for arg in node.args.args:
+ num_args = len(node.args.args)
+ num_defaults = len(node.args.defaults)
+ first_default_index = num_args - num_defaults
+
+ for idx, arg in enumerate(node.args.args):
arg_name = arg.arg
arg_type = ast.unparse(arg.annotation) if arg.annotation else
"Any"
if arg_name != "self":
- args.append({arg_name: arg_type})
+ has_default = idx >= first_default_index
+ args.append({arg_name: arg_type, "has_default":
has_default})
Review Comment:
The `parameters` entries are now shaped as a dict with a dynamic key for the
param name plus a reserved `has_default` key. This forces downstream code to
iterate `parameter.items()` and special-case `parameter_key ==
\"has_default\"`, which is brittle and harder to extend (and also makes
collisions with a real param named `has_default` difficult to reason about).
Consider switching to a stable structure like `{ \"name\": ..., \"type\": ...,
\"has_default\": ... }` for each parameter so consumers can access fields
directly without item-iteration + skips.
```suggestion
args.append(
{"name": arg_name, "type": arg_type, "has_default":
has_default}
)
```
##########
airflow-ctl/tests/airflow_ctl/ctl/test_cli_config.py:
##########
@@ -554,3 +539,60 @@ def test_apply_datamodel_defaults_other_datamodel(self):
# Should return params unchanged for other datamodels
assert result == params, "Params should be unchanged for
non-TriggerDAGRunPostBody datamodels"
+
+ def test_positional_args_for_required_params(self):
+ """Test that required non-boolean params become positional args and
optional params remain flags."""
+ command_factory = CommandFactory(file_path="")
+
+ # Required non-boolean → positional (no -- prefix)
+ positional_arg = command_factory._create_arg(
+ arg_flags=("connection_id",),
+ arg_type=str,
+ arg_help="Connection ID",
+ arg_action=None,
+ )
+ assert positional_arg.flags == ("connection_id",)
+ assert "dest" not in positional_arg.kwargs
+
+ # Optional → flag (-- prefix)
+ optional_arg = command_factory._create_arg(
+ arg_flags=("--description",),
+ arg_type=str,
+ arg_help="Description",
+ arg_action=None,
+ )
+ assert optional_arg.flags == ("--description",)
+
+ def test_has_default_detection_in_ast_parsing(self):
+ """Test that AST parsing correctly detects which params have
defaults."""
+ from textwrap import dedent
+
+ temp_file = "test_has_default.py"
+ with open(temp_file, "w") as f:
+ f.write(
+ dedent("""
+ class TestOperations(BaseOperations):
+ def get(self, required_id: str) -> str |
ServerResponseError:
+ pass
+ def search(self, query: str, limit: int = 10, offset: int
= 0) -> str | ServerResponseError:
+ pass
+ """)
+ )
+
+ try:
+ command_factory = CommandFactory(file_path=temp_file)
+ for op in command_factory.operations:
+ if op["name"] == "get":
+ param = op["parameters"][0]
+ assert param.get("has_default") is False
+ elif op["name"] == "search":
+ query_param = op["parameters"][0]
+ assert query_param.get("has_default") is False
+ limit_param = op["parameters"][1]
+ assert limit_param.get("has_default") is True
+ offset_param = op["parameters"][2]
+ assert offset_param.get("has_default") is True
+ finally:
+ import os
+
+ os.remove(temp_file)
Review Comment:
This test writes a fixed filename (`test_has_default.py`) into the working
directory, which can conflict under parallel test runs or when the CWD is not
writable. Use pytest’s `tmp_path`/`tmp_path_factory` (or
`tempfile.NamedTemporaryFile`) to create an isolated temporary file path, and
open it with an explicit encoding (e.g. `encoding=\"utf-8\"`) to match the
production file-read behavior.
```suggestion
def test_has_default_detection_in_ast_parsing(self, tmp_path):
"""Test that AST parsing correctly detects which params have
defaults."""
from textwrap import dedent
temp_file = tmp_path / "test_has_default.py"
temp_file.write_text(
dedent("""
class TestOperations(BaseOperations):
def get(self, required_id: str) -> str |
ServerResponseError:
pass
def search(self, query: str, limit: int = 10, offset:
int = 0) -> str | ServerResponseError:
pass
"""),
encoding="utf-8",
)
command_factory = CommandFactory(file_path=str(temp_file))
for op in command_factory.operations:
if op["name"] == "get":
param = op["parameters"][0]
assert param.get("has_default") is False
elif op["name"] == "search":
query_param = op["parameters"][0]
assert query_param.get("has_default") is False
limit_param = op["parameters"][1]
assert limit_param.get("has_default") is True
offset_param = op["parameters"][2]
assert offset_param.get("has_default") is True
```
--
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]
