Copilot commented on code in PR #38440:
URL: https://github.com/apache/superset/pull/38440#discussion_r2897231606
##########
superset/views/base.py:
##########
@@ -477,7 +477,13 @@ def cached_common_bootstrap_data( # pylint:
disable=unused-argument
)
if isinstance(locale, Locale):
- language = locale.language
+ # Use language + territory to support locales like zh_TW.
+ # locale.language alone strips the territory (e.g. zh_TW → zh).
+ language = (
+ f"{locale.language}_{locale.territory}"
+ if locale.territory
+ else locale.language
+ )
Review Comment:
Instead of manually rebuilding the identifier from
`locale.language`/`locale.territory`, consider using `str(locale)` (or Babel's
dedicated formatter if available). This matches existing usage in the codebase
(eg `str(get_locale())`) and avoids dropping other locale components (eg
script/variant) that `territory` alone won’t capture.
```suggestion
language = str(locale)
```
##########
superset/views/base.py:
##########
@@ -477,7 +477,13 @@ def cached_common_bootstrap_data( # pylint:
disable=unused-argument
)
if isinstance(locale, Locale):
- language = locale.language
+ # Use language + territory to support locales like zh_TW.
+ # locale.language alone strips the territory (e.g. zh_TW → zh).
+ language = (
+ f"{locale.language}_{locale.territory}"
+ if locale.territory
+ else locale.language
+ )
elif isinstance(locale, str):
language = locale
Review Comment:
This change will cause territory-specific locales (eg `zh_TW`) to be sent to
the frontend. The frontend locale loader currently has no `zh_TW` entry in
`src/hooks/useLocale.ts` (`LOCALE_MAPPING`), and it doesn’t import a Day.js
`zh-tw` locale either, so date/time localization may silently fall back to
defaults even though translations improve. Consider updating the frontend
locale mapping/imports alongside this change (or adding a backend fallback only
for date/time libs if that’s the intended behavior).
```suggestion
# Normalize to base language (e.g. 'zh' from 'zh_TW') to match
frontend expectations.
language = locale.language
elif isinstance(locale, str):
# Normalize string locales like 'en_US' or 'zh-TW' to their base
language code.
language = locale.split("_")[0].split("-")[0]
```
##########
superset/views/base.py:
##########
@@ -477,7 +477,13 @@ def cached_common_bootstrap_data( # pylint:
disable=unused-argument
)
if isinstance(locale, Locale):
- language = locale.language
+ # Use language + territory to support locales like zh_TW.
+ # locale.language alone strips the territory (e.g. zh_TW → zh).
+ language = (
+ f"{locale.language}_{locale.territory}"
+ if locale.territory
+ else locale.language
+ )
Review Comment:
The locale formatting behavior of `cached_common_bootstrap_data` is changing
(eg `Locale('zh', territory='TW')` should now produce `zh_TW`), but there
doesn’t appear to be unit test coverage for this function in
`superset/views/base.py` (existing tests cover theme helpers only). Adding a
unit test for territory-preserving locales would help prevent regressions, per
the repo’s preference for unit tests.
--
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]
