pierrejeambrun commented on code in PR #53501:
URL: https://github.com/apache/airflow/pull/53501#discussion_r2225936278
##########
airflow-core/src/airflow/api/common/delete_dag.py:
##########
@@ -63,10 +64,20 @@ def delete_dag(dag_id: str, keep_records_in_log: bool =
True, session: Session =
if dag is None:
raise DagNotFound(f"Dag id {dag_id} not found")
- count = 0
+ """
+ To ensure the TaskInstance and DagRun model is deleted before
+ each of the model DagVersion and BackFill respectively.
+ """
Review Comment:
Nit:
Most of the time this is reserved for docstring. For simple multilines
comment we favor this
```suggestion
# To ensure the TaskInstance and DagRun model is deleted before
# each of the model DagVersion and BackFill respectively.
```
##########
airflow-core/src/airflow/api/common/delete_dag.py:
##########
@@ -63,10 +64,20 @@ def delete_dag(dag_id: str, keep_records_in_log: bool =
True, session: Session =
if dag is None:
raise DagNotFound(f"Dag id {dag_id} not found")
- count = 0
+ """
+ To ensure the TaskInstance and DagRun model is deleted before
+ each of the model DagVersion and BackFill respectively.
+ """
+ models_for_deletion = [TaskInstance, DagRun] + [
+ model
+ for model in get_sqla_model_classes()
+ if (not keep_records_in_log or model.__name__ != "Log")
+ and model.__name__ not in ["TaskInstance", "DagRun"]
+ ]
- for model in get_sqla_model_classes():
- if hasattr(model, "dag_id") and (not keep_records_in_log or
model.__name__ != "Log"):
+ count = 0
+ for model in models_for_deletion:
+ if hasattr(model, "dag_id"):
Review Comment:
Nit:
We split in two the check `if hasattr(model, "dag_id") and (not
keep_records_in_log or model.__name__ != "Log"):`
I think all of it should go above in the `models_for_deletion` map
comprehension, or none of it should. Since the comprehension will be quite hard
to read, I would say to keep the check the same here and only do:
```python
models_for_deletion = [TaskInstance, DagRun] + [
model
for model in get_sqla_model_classes()
if model.__name__ not in ["TaskInstance", "DagRun"]
]
```
--
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]
