jscheffl commented on PR #59876: URL: https://github.com/apache/airflow/pull/59876#issuecomment-3899993391
> This is a change for no difference to my mind. Honestly I don't see the point of the change. I tried to follow your request to adjust. I had a static class instance before and went into a singleton pattern. `global` is evil and we should follow best practices and good standards. I would also follow any further constructive comment on it. Also please see that compared to `SUPERVISOR_COMMS.send()` now we have `supervisor_send()` as call which rads much cleaner. The upper-casing directly jumps into my eyes as something "evil". Yes, technically a singleton is also a global but with the construct it allows a controlled access and makes clear that access is validated. All calls can trust that the method will fail with a clear error. If the codebase grows we will lose control over time where the global is spread in the context. I saw multiple times when testing changes here that a pytest failed on the global set to None and needed to search where some other test messed-up the global. I put effort in this because I'd prefer to have a codebase where we are clean and can be proud of, local optimizations with `global`... yes might be working but leave a smell of not good practice and even if the change is in your view technically neutral it is an improvement to remove two usages of `global`. -- 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]
