[GitHub] [superset] ktmud commented on a change in pull request #13020: fix: Let superset configure the correct time display according to locale

[GitBox <gi...@apache.org>]

ktmud commented on a change in pull request #13020:
URL: https://github.com/apache/superset/pull/13020#discussion_r572722243



##########
File path: superset/models/helpers.py
##########
@@ -425,7 +425,19 @@ def changed_on_utc(self) -> str:
 
     @property
     def changed_on_humanized(self) -> str:
-        return humanize.naturaltime(datetime.now() - self.changed_on)
+        try:
+            from superset import conf
+            from flask_babel import get_locale
+            langs = conf.get("LANGUAGES", None)
+            locale = str(get_locale())
+
+            if "_" in locale:
+                humanize.i18n.activate(locale)
+            elif langs is not None:
+                humanize.i18n.activate("{}_{}".format(locale,
+                                                      langs[locale]["flag"].upper()))

Review comment:
       I'd assume this locale setting is global. You might want to call
   
   ```python
   humanize.i18n.deactivate()
   ```
   
   to avoid localization being polluted across different request context.
   
   Maybe create a helper function in `superset/utils/i18n.py`?
   
   ```python
   def localized_naturaltime(time: datetime, locale: str) -> str:
      humanize.i18n.activate(locale)
      naturalized = humanize.naturaltime(time)
      humanize.i18n.deactivate(locale)
      return naturalized
   ```
   

##########
File path: superset-frontend/src/preamble.ts
##########
@@ -40,7 +40,7 @@ if (typeof window !== 'undefined') {
   if (bootstrapData.common && bootstrapData.common.language_pack) {
     const languagePack = bootstrapData.common.language_pack;
     configure({ languagePack });
-    moment.locale(bootstrapData.common.locale);
+    moment.locale(bootstrapData.common.moment_locale);

Review comment:
       AFAIK we don't load additional locales for `moment` so this is probably not needed. 
   
   Let's leave this frontend change to another PR since there might be more places you need to update.

##########
File path: superset/views/base.py
##########
@@ -327,6 +329,17 @@ def common_bootstrap_payload() -> Dict[str, Any]:
     }
 
 
+def get_moment_locale(locale):
+    moment_locale = locale
+    if "_" in moment_locale:
+        moment_locale = moment_locale.split("_")[0]
+
+    flag = conf.get("LANGUAGES").get(locale).get("flag")

Review comment:
       `flag` is for the country flag icon, which doesn't seem right to make it as a dependency for locale inference.
   
   You should probably use [`babel.core.negotiate_locale`](http://babel.pocoo.org/en/latest/api/core.html#babel.core.negotiate_locale) to select from `humanize`'s [supported locales](https://github.com/jmoiron/humanize/tree/master/src/humanize/locale):
   
   ```python
   locale = negotiate_locale([str(get_locale())], ['de_DE', 'es_ES', 'fa_IR', 'fi_FL'...])
   ```
   
   It seems `humanize` doesn't export the list of supported locales so you may need to manually define this list as a constant.
   

##########
File path: superset/views/base.py
##########
@@ -327,6 +329,17 @@ def common_bootstrap_payload() -> Dict[str, Any]:
     }
 
 
+def get_moment_locale(locale):

Review comment:
       Feel free to create a `superset/utils/i18n.py` file and move this function over.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org