TiDeane commented on code in PR #38502:
URL: https://github.com/apache/airflow/pull/38502#discussion_r1541517912
##########
airflow/utils/log/task_handler_with_custom_formatter.py:
##########
@@ -45,7 +45,7 @@ def set_context(self, ti) -> None:
:param ti:
:return:
"""
- if ti.raw or self.formatter is None:
+ if ti.raw or self.formatter is None or self.prefix_jinja_template is
not None:
Review Comment:
Thank you for the quick response! I updated the tests on
`test_task_handler_with_custom_formatter.py`, and added a comment on
`TaskHandlerWithCustomFormatter`'s initial if statement.
I changed
[`test_custom_formatter_custom_format_not_affected_by_config`](https://github.com/apache/airflow/blob/main/tests/utils/test_task_handler_with_custom_formatter.py#L87)
to make it call `logging_mixin`'s `set_context` three times instead of one. As
expected, even after adding more calls of `set_context` (or doing `assert
expected_format == handler.formatter._fmt` in-between each call, which isn't
included in the final version) the final result is correct, that is, only a
single prefix is added.
Along with this, I also made it so that both this test and
[`test_custom_formatter_default_format`](https://github.com/apache/airflow/blob/main/tests/utils/test_task_handler_with_custom_formatter.py#L81)
reset the handler's formatter at the end of their execution. This had to be
changed, because previously the tests were passing by pure luck:
- After `test_custom_formatter_default_format`, an empty prefix (":") would
be added to `handler.formatter._fmt`;
- When `test_custom_formatter_custom_format_not_affected_by_config` was
called, `expected_value` became `f"{prefix}:{handler.formatter._fmt}"`, which
had an extra ":" because of the previously added prefix;
- Finally, the `set_context` call would add an extra prefix on top of the
previous one (the original bug), which would result in the given prefix with
two ":", which was equal to the expected value and resulted in the test passing.
Now it should work properly, and it also verifies that repeated calls of
`set_context` don't add repeated prefixes.
--
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
