xBis7 commented on code in PR #72458:
URL: https://github.com/apache/airflow/pull/72458#discussion_r3978976217


##########
shared/observability/src/airflow_shared/observability/common.py:
##########
@@ -30,6 +31,20 @@
 log = structlog.getLogger(__name__)
 
 
+def expand_dag_tags(tag_names: Iterable[str]) -> dict[str, str]:
+    """Expand DAG tag attributes into key-value pairs."""
+    result: dict[str, str] = {}
+    for name in tag_names:
+        key, _, value = name.partition(":")
+        result[key] = value
+    return result
+
+
+def build_dag_tags(tag_names: Iterable[str]) -> dict[str, str]:

Review Comment:
   This function doesn't do anything other than call `expand_dag_tags` while 
both functions have the same signature (params, return type). So why keep both?
   
   I would understand if `build_dag_tags` had some extra step(s) other than 
`expand_dag_tags` and it was easier to unit test the logic by keeping it 
separated. But as it currently is, I don't see a reason to keep both.
   
   Also the docstring is confusing. It says
   ```python
   """Build observability tags from DAG tags."""
   ```
   
   and then just calls `expand_dag_tags` which has a docstring
   ```python
   """Expand DAG tag attributes into key-value pairs."""
   ```
   
   `expand_dag_tags` both expands the tags and populates ("builds") a dict.
   
   



-- 
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]

Reply via email to