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: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org