uranusjr commented on code in PR #32520:
URL: https://github.com/apache/airflow/pull/32520#discussion_r1266235964
##########
airflow/models/dag.py:
##########
@@ -473,6 +475,8 @@ def __init__(
validate_key(dag_id)
self._dag_id = dag_id
+ self._display_name = display_name or dag_id
Review Comment:
Since we already have fallback logic in the property, the `or dag_id` part
is not needed. It’s better to be able to store the arguments losslessly.
##########
airflow/models/taskinstance.py:
##########
@@ -624,6 +626,14 @@ def operator_name(self) -> str | None:
"""@property: use a more friendly display name for the operator, if
set."""
return self.custom_operator_name or self.operator
+ @hybrid_property
+ def display_name(self) -> str:
+ return self._display_name or self.task_id
+
+ @display_name.setter
+ def display_name(self, value):
+ self._display_name = value
Review Comment:
Same, not needed.
##########
airflow/models/dag.py:
##########
@@ -1222,6 +1226,14 @@ def access_control(self):
def access_control(self, value):
self._access_control = DAG._upgrade_outdated_dag_access_control(value)
+ @property
+ def display_name(self) -> str:
+ return self._display_name or self._dag_id
+
+ @display_name.setter
+ def display_name(self, value):
+ self._display_name = value
Review Comment:
I don’t think we need a setter?
##########
airflow/models/dag.py:
##########
@@ -1222,6 +1226,14 @@ def access_control(self):
def access_control(self, value):
self._access_control = DAG._upgrade_outdated_dag_access_control(value)
+ @property
+ def display_name(self) -> str:
+ return self._display_name or self._dag_id
+
+ @display_name.setter
+ def display_name(self, value):
+ self._display_name = value
Review Comment:
I don’t think we need a setter?
##########
airflow/models/dag.py:
##########
@@ -473,6 +475,8 @@ def __init__(
validate_key(dag_id)
self._dag_id = dag_id
+ self._display_name = display_name or dag_id
Review Comment:
Since we already have fallback logic in the property, the `or dag_id` part
is not needed. It’s better to be able to store the arguments losslessly.
##########
airflow/models/baseoperator.py:
##########
@@ -915,6 +919,7 @@ def __init__(
self.doc_yaml = doc_yaml
self.doc_rst = doc_rst
self.doc = doc
+ self.display_name = display_name or self.task_id
Review Comment:
Should this also use a property?
##########
airflow/models/dag.py:
##########
@@ -441,6 +442,7 @@ def __init__(
owner_links: dict[str, str] | None = None,
auto_register: bool = True,
fail_stop: bool = False,
+ display_name: str = None,
Review Comment:
```suggestion
display_name: str = "",
```
We can get rid of the `None` case entirely. Same for all other classes.
##########
airflow/models/dag.py:
##########
@@ -3380,6 +3393,8 @@ class DagModel(Base):
processor_subdir = Column(String(2000), nullable=True)
# String representing the owners
owners = Column(String(2000))
+ # Display name of the dag
+ display_name = Column(String(2000))
Review Comment:
This also needs the fallback mechanism like TaskInstance
--
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]
