uranusjr commented on code in PR #34964:
URL: https://github.com/apache/airflow/pull/34964#discussion_r1366727096
##########
airflow/utils/log/logging_mixin.py:
##########
@@ -68,14 +68,43 @@ class LoggingMixin:
"""Convenience super-class to have a logger configured with the class
name."""
_log: logging.Logger | None = None
+ _parent_logger: str | None = None
+ _logger_name: str | None = None
def __init__(self, context=None):
self._set_context(context)
@staticmethod
- def _get_log(obj: Any, clazz: type[_T]) -> Logger:
+ def _generate_logger_name(obj: Any, clazz: type[_T]) -> str:
+ """Generate a logger name.
+
+ Generate logger name based on:
+ - The class `_parent_logger` attribute.
+ If set to None, only the `_logger_name` attribute will be used to
create a logger name.
+ - The class `_logger_name` attribute.
+ If set to None, fallback to class.__module__ and class.__name__.
When the attribute is an empty
+ string, the returned logger name is equal to the parent logger
name.
+
+ When there is no `_parent_logger` and an empty string as
`_logger_name`, the returned logger name is
+ an empty string.
+ """
+ return ".".join(
+ filter(
+ None,
+ [
+ obj._parent_logger,
+ obj._logger_name
+ if obj._logger_name is not None
+ else f"{clazz.__module__}.{clazz.__name__}",
+ ],
Review Comment:
Is this joining logic correct? If parent logger is set but logger name is
not, this would include the module name twice. I guess you can argue that’s not
what the two should be combined, but that seems a bit too easy to get wrong.
Would the same be achieved by defaulting parent logger name to module name, and
defaulting the logger name to the class name?
##########
airflow/utils/log/logging_mixin.py:
##########
@@ -68,14 +68,43 @@ class LoggingMixin:
"""Convenience super-class to have a logger configured with the class
name."""
_log: logging.Logger | None = None
+ _parent_logger: str | None = None
+ _logger_name: str | None = None
def __init__(self, context=None):
self._set_context(context)
@staticmethod
- def _get_log(obj: Any, clazz: type[_T]) -> Logger:
+ def _generate_logger_name(obj: Any, clazz: type[_T]) -> str:
+ """Generate a logger name.
+
+ Generate logger name based on:
+ - The class `_parent_logger` attribute.
+ If set to None, only the `_logger_name` attribute will be used to
create a logger name.
+ - The class `_logger_name` attribute.
+ If set to None, fallback to class.__module__ and class.__name__.
When the attribute is an empty
+ string, the returned logger name is equal to the parent logger
name.
+
+ When there is no `_parent_logger` and an empty string as
`_logger_name`, the returned logger name is
+ an empty string.
+ """
+ return ".".join(
+ filter(
+ None,
+ [
+ obj._parent_logger,
+ obj._logger_name
+ if obj._logger_name is not None
+ else f"{clazz.__module__}.{clazz.__name__}",
+ ],
Review Comment:
Is this joining logic correct? If parent logger is set but logger name is
not, this would include the module name twice. I guess you can argue that’s not
what the two should be combined, but that seems a bit too easy to get wrong.
Would the same be achieved by defaulting parent logger name to module name, and
defaulting the logger name to the class name?
--
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: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
