Copilot commented on code in PR #58838:
URL: https://github.com/apache/airflow/pull/58838#discussion_r2572964992
##########
airflow-core/tests/unit/listeners/lifecycle_listener.py:
##########
@@ -17,27 +17,35 @@
from __future__ import annotations
+from dataclasses import dataclass
+from functools import cache
from typing import Any
from airflow.listeners import hookimpl
-started_component: Any = None
-stopped_component: Any = None
+
+@dataclass
+class ListenerState:
+ started_component: Any = None
+ stopped_component: Any = None
+
+
+@cache
+def get_listener_state() -> ListenerState:
+ return ListenerState()
@hookimpl
def on_starting(component):
- global started_component
- started_component = component
+ get_listener_state().started_component = component
@hookimpl
def before_stopping(component):
- global stopped_component
- stopped_component = component
+ get_listener_state().stopped_component = component
def clear():
- global started_component, stopped_component
- started_component = None
- stopped_component = None
+ state = get_listener_state()
+ state.started_component = None
+ state.stopped_component = None
Review Comment:
[nitpick] The local variable name `state` on line 49 could be confused with
the listener state itself. Consider renaming to `listener_state` or `listener`
for better clarity and consistency with the function being called.
```python
def clear():
listener_state = get_listener_state()
listener_state.started_component = None
listener_state.stopped_component = None
```
```suggestion
listener_state = get_listener_state()
listener_state.started_component = None
listener_state.stopped_component = None
```
##########
airflow-core/tests/unit/listeners/full_listener.py:
##########
@@ -17,45 +17,53 @@
# under the License.
from __future__ import annotations
+from dataclasses import dataclass, field
+from functools import cache
from typing import Any
from airflow.listeners import hookimpl
from airflow.utils.state import TaskInstanceState
-started_component: Any = None
-stopped_component: Any = None
-state: list[Any] = []
+
+@dataclass
+class ListenerState:
+ started_component: Any = None
+ stopped_component: Any = None
+ state: list[Any] = field(default_factory=list)
+
+
+@cache
+def get_listener_state() -> ListenerState:
+ return ListenerState()
@hookimpl
def on_starting(component):
- global started_component
- started_component = component
+ get_listener_state().started_component = component
@hookimpl
def before_stopping(component):
- global stopped_component
- stopped_component = component
+ get_listener_state().stopped_component = component
@hookimpl
def on_task_instance_running(previous_state, task_instance):
- state.append(TaskInstanceState.RUNNING)
+ get_listener_state().state.append(TaskInstanceState.RUNNING)
@hookimpl
def on_task_instance_success(previous_state, task_instance):
- state.append(TaskInstanceState.SUCCESS)
+ get_listener_state().state.append(TaskInstanceState.SUCCESS)
@hookimpl
def on_task_instance_failed(previous_state, task_instance, error: None | str |
BaseException):
- state.append(TaskInstanceState.FAILED)
+ get_listener_state().state.append(TaskInstanceState.FAILED)
def clear():
- global started_component, stopped_component, state
- started_component = None
- stopped_component = None
- state = []
+ state = get_listener_state()
+ state.started_component = None
+ state.stopped_component = None
+ state.state = []
Review Comment:
[nitpick] The local variable name `state` on line 66 shadows the attribute
name `state.state` on line 69, which may cause confusion. Consider renaming the
local variable to `listener_state` or `listener` for better clarity.
```python
def clear():
listener_state = get_listener_state()
listener_state.started_component = None
listener_state.stopped_component = None
listener_state.state = []
```
```suggestion
listener_state = get_listener_state()
listener_state.started_component = None
listener_state.stopped_component = None
listener_state.state = []
```
--
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]
