jscheffl commented on code in PR #39006:
URL: https://github.com/apache/airflow/pull/39006#discussion_r1567847175
##########
airflow/www/static/js/utils/index.ts:
##########
@@ -185,6 +185,34 @@ const toSentenceCase = (camelCase: string): string => {
return "";
};
+const addColorKeyword = (
Review Comment:
If the function is separated-out into a utility, can you use this
opportunity to add a unit test to airflow/www/static/js/utils/index.test.ts as
well?
##########
airflow/config_templates/config.yml:
##########
@@ -979,6 +979,22 @@ logging:
type: boolean
example: ~
default: "True"
+ color_log_error_keywords:
Review Comment:
Not to be too "picky" but you name the config arguments "error" and
"warning" - mainly because you apply typical red/yellow colors as highlight. So
in most cases this really apply to errors and warnings. And this is common. But
functional wise it is not linked to this. Did you consider naming the config
keys `color_log_red_highlight` and `color_log_yellow_highlight`? (and of course
keeping error/warn as default values) It would be a bit closer to what actually
is configured.
```suggestion
color_log_red_highlight_keywords:
```
##########
airflow/www/static/js/utils/index.ts:
##########
@@ -185,6 +185,34 @@ const toSentenceCase = (camelCase: string): string => {
return "";
};
+const addColorKeyword = (
+ parsedLine: string,
+ errorKeywords: string[],
+ warningKeywords: string[]
+): string => {
+ const lowerParsedLine = parsedLine.toLowerCase();
+ const containsError = errorKeywords.some((keyword) =>
+ lowerParsedLine.includes(keyword)
+ );
+ const bold = (line: string) => `\x1b[1m${line}\x1b[0m`;
+ const red = (line: string) => `\x1b[31m${line}\x1b[39m`;
Review Comment:
As the log might be long and this operation is potentially long-running -
could you merge the bold+[red|yellow] operations into one to save some CPU?
##########
airflow/www/static/js/utils/index.ts:
##########
@@ -185,6 +185,34 @@ const toSentenceCase = (camelCase: string): string => {
return "";
};
+const addColorKeyword = (
Review Comment:
Alongside, the name of the function might be mis-leading for first-time
readers. Could you rename it to `coloByKeywords()` or `highlightByKeywords()`?
--
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]
