ashb commented on code in PR #60258:
URL: https://github.com/apache/airflow/pull/60258#discussion_r2671829482
##########
shared/logging/src/airflow_shared/logging/percent_formatter.py:
##########
@@ -53,17 +53,22 @@ def __getitem__(self, key):
# If there is no callsite info (often for stdout/stderr), show the
same sort of thing that stdlib
# logging would
#
https://github.com/python/cpython/blob/d3c888b4ec15dbd7d6b6ef4f15b558af77c228af/Lib/logging/__init__.py#L1652C34-L1652C48
- if key == "lineno":
- return self.event.get("lineno", 0)
- if key == "filename":
- return self.event.get("filename", "(unknown file)")
- if key == "funcName":
- return self.event.get("funcName", "(unknown function)")
-
if key in PercentFormatRender.callsite_parameters:
- return
self.event.get(PercentFormatRender.callsite_parameters[key].value)
+ val =
self.event.get(PercentFormatRender.callsite_parameters[key].value)
+ # Return appropriate default based on the parameter type
+ if val is None:
+ if key == "lineno":
+ return 0
+ if key in ("process", "thread"):
+ return 0
+ if key == "filename":
+ return "(unknown file)"
+ if key == "funcName":
+ return "(unknown function)"
+ return "(unknown)"
+ return val
if key == "name":
- return self.event.get("logger") or self.event.get("logger_name")
+ return self.event.get("logger") or self.event.get("logger_name")
or "(unknown)"
Review Comment:
```suggestion
return self.event.get("logger") or self.event.get("logger_name",
"(unknown)")
```
##########
shared/logging/src/airflow_shared/logging/percent_formatter.py:
##########
@@ -53,17 +53,22 @@ def __getitem__(self, key):
# If there is no callsite info (often for stdout/stderr), show the
same sort of thing that stdlib
# logging would
#
https://github.com/python/cpython/blob/d3c888b4ec15dbd7d6b6ef4f15b558af77c228af/Lib/logging/__init__.py#L1652C34-L1652C48
- if key == "lineno":
- return self.event.get("lineno", 0)
- if key == "filename":
- return self.event.get("filename", "(unknown file)")
- if key == "funcName":
- return self.event.get("funcName", "(unknown function)")
-
if key in PercentFormatRender.callsite_parameters:
- return
self.event.get(PercentFormatRender.callsite_parameters[key].value)
+ val =
self.event.get(PercentFormatRender.callsite_parameters[key].value)
+ # Return appropriate default based on the parameter type
+ if val is None:
+ if key == "lineno":
+ return 0
+ if key in ("process", "thread"):
+ return 0
+ if key == "filename":
+ return "(unknown file)"
+ if key == "funcName":
+ return "(unknown function)"
+ return "(unknown)"
+ return val
Review Comment:
Honestly this is now a bit hard to follow:
```suggestion
if key in {"lineno", "process" "thread"}:
return self.event.get(key, 0)
if key == "filename":
return self.event.get("filename", "(unknown file)")
if key == "funcName":
return self.event.get("funcName", "(unknown function)")
if key in PercentFormatRender.callsite_parameters:
return
self.event.get(PercentFormatRender.callsite_parameters[key].value, "(unknown)")
```
##########
shared/logging/src/airflow_shared/logging/percent_formatter.py:
##########
@@ -85,7 +90,8 @@ def __getitem__(self, key):
return ""
return self.level_styles.get(self.event.get("level",
self.method_name), "")
- return self.event[key]
+ val = self.event.get(key)
+ return val if val is not None else ""
Review Comment:
I'm not so sure about this one -- it would mean that `"%(x)r"` would show
`""` instead of `None`.
##########
shared/logging/tests/logging/test_percent_formatter.py:
##########
@@ -29,3 +29,21 @@ def test_no_callsite(self):
formatted = fmter(mock.Mock(name="Logger"), "info", {"event": "our
msg"})
assert formatted == "(unknown file):0 our msg"
+
+ def test_percent_formatter_handles_none_values(self):
+ """Test that PercentFormatRender handles None values gracefully."""
+ fmter = PercentFormatRender("%(filename)s:%(lineno)d %(message)s")
+
+ # Mock logger and event_dict with None values
+ logger = mock.Mock(name="Logger")
+ event_dict = {
+ "event": "test message",
+ "filename": None,
+ "lineno": None,
+ }
Review Comment:
Is this accurately representing the event dict that is created, or would it
be this?
```py
event_dict = {
"event": "test message",
}
```
--
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]
