korbit-ai[bot] commented on code in PR #34118: URL: https://github.com/apache/superset/pull/34118#discussion_r2196704194
########## superset-frontend/scripts/po2json.sh: ########## @@ -22,14 +22,16 @@ set -e +export NODE_NO_WARNINGS=1 Review Comment: ### Overly broad warning suppression <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? Suppressing all Node.js warnings could hide important runtime issues unrelated to the translation process. ###### Why this matters While the intent is to clean the console output, this global warning suppression could mask critical warnings about memory leaks, deprecation notices, or other runtime problems that need attention. ###### Suggested change ∙ *Feature Preview* Instead of suppressing all Node.js warnings, consider using a more targeted approach by specifically handling the warnings you want to suppress. For example, you could redirect stderr for just the po2json command: ```bash po2json --domain superset --format jed1.x --fuzzy $file $filename.json 2>/dev/null ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4d15561f-5803-4d04-b0ec-fa1f1513f44e/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4d15561f-5803-4d04-b0ec-fa1f1513f44e?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4d15561f-5803-4d04-b0ec-fa1f1513f44e?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4d15561f-5803-4d04-b0ec-fa1f1513f44e?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4d15561f-5803-4d04-b0ec-fa1f1513f44e) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:689c6951-1d51-4d94-b41a-dc98f0b1f735 --> [](689c6951-1d51-4d94-b41a-dc98f0b1f735) ########## superset-frontend/packages/superset-ui-core/src/translation/Translator.ts: ########## @@ -86,20 +86,35 @@ export default class Translator { } translate(input: string, ...args: unknown[]): string { - return this.i18n.translate(input).fetch(...args); + try { + return this.i18n.translate(input).fetch(...args); + } catch (err) { + logging.warn(`Translation failed for key "${input}" with args:`, args); + logging.warn(err); Review Comment: ### Inefficient Multiple Logging Calls <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? Multiple consecutive logging calls for the same error event creates unnecessary I/O operations ###### Why this matters Each logging operation is potentially an I/O operation that can impact performance, especially in error scenarios where multiple translations might fail ###### Suggested change ∙ *Feature Preview* Combine the log messages into a single operation: ```typescript logging.warn(`Translation failed for key "${input}" with args:`, args, '\nError:', err); ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d8b5888b-7020-4cd2-97bb-d491b24ecff6/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d8b5888b-7020-4cd2-97bb-d491b24ecff6?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d8b5888b-7020-4cd2-97bb-d491b24ecff6?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d8b5888b-7020-4cd2-97bb-d491b24ecff6?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d8b5888b-7020-4cd2-97bb-d491b24ecff6) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:3b30cd60-7a66-49c2-84a6-309ea70d4df2 --> [](3b30cd60-7a66-49c2-84a6-309ea70d4df2) ########## superset-frontend/packages/superset-ui-core/src/translation/Translator.ts: ########## @@ -86,20 +86,35 @@ } translate(input: string, ...args: unknown[]): string { - return this.i18n.translate(input).fetch(...args); + try { + return this.i18n.translate(input).fetch(...args); + } catch (err) { + logging.warn(`Translation failed for key "${input}" with args:`, args); + logging.warn(err); + return input; + } } translateWithNumber(key: string, ...args: unknown[]): string { - const [plural, num, ...rest] = args; - if (typeof plural === 'number') { + try { + const [plural, num, ...rest] = args; + if (typeof plural === 'number') { + return this.i18n + .translate(key) + .ifPlural(plural, key) + .fetch(plural, num, ...args); + } return this.i18n .translate(key) - .ifPlural(plural, key) - .fetch(plural, num, ...args); + .ifPlural(num as number, plural as string) + .fetch(...rest); + } catch (err) { + logging.warn( + `Plural translation failed for key "${key}" with args:`, + args, + ); + logging.warn(err); + return key; } Review Comment: ### Duplicate Error Logging <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? Similar to the first translate method, the error logging is split into two separate warn statements. ###### Why this matters Multiple log statements for the same error make it harder to track related log entries and increase log verbosity. ###### Suggested change ∙ *Feature Preview* Combine the error logging into a single statement: ```typescript catch (err) { logging.warn(`Plural translation failed for key "${key}" with args:`, args, err); return key; } ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/dd060d9b-26ec-474d-8c0f-243dd28f8af6/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/dd060d9b-26ec-474d-8c0f-243dd28f8af6?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/dd060d9b-26ec-474d-8c0f-243dd28f8af6?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/dd060d9b-26ec-474d-8c0f-243dd28f8af6?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/dd060d9b-26ec-474d-8c0f-243dd28f8af6) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:f2b3e414-c720-4420-b6de-94fda99cb3ae --> [](f2b3e414-c720-4420-b6de-94fda99cb3ae) -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
