You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2021/02/09 07:14:06 UTC

[GitHub] [superset] cdmikechen opened a new pull request #13020: fix: Let superset configure the correct time display according to locale

cdmikechen opened a new pull request #13020:
URL: https://github.com/apache/superset/pull/13020


   ### SUMMARY
   Let superset configure the correct time display according to locale. Now superset can't display the correct time format according to locale
   
   ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
   ![chartlist](https://user-images.githubusercontent.com/12069428/107328128-32f87500-6ae9-11eb-9d6f-ee3e3448642f.jpg)
   
   
   ### TEST PLAN
   Can view the results in a browser
   
   ### ADDITIONAL INFORMATION
   <!--- Check any relevant boxes with "x" -->
   <!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
   - [ ] Has associated issue:
   - [x] Changes UI
   - [ ] Requires DB Migration.
   - [ ] Confirm DB Migration upgrade and downgrade tested.
   - [ ] Introduces new feature or API
   - [ ] Removes existing feature or API
   


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

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

Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on a change in pull request #13020:
URL: https://github.com/apache/superset/pull/13020#discussion_r573386638



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

Review comment:
       @cdmikechen 
   十分感谢您的代码贡献. 如果我们的目标是变更REST返回的时间. 看起来我们只需要在message.po里面加入一些翻译内容就可以.
   ```
   "%s from now"
   "%s ago"
   "a moment"
    "now"
   ```
   您可以参考 `humanize` 的代码
   https://github.com/jmoiron/humanize/blob/2d4143e0aecb719a376dc757b439fc7e41b0d773/src/humanize/time.py#L190-L223
   




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

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

Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #13020:
URL: https://github.com/apache/superset/pull/13020#discussion_r573137765



##########
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:
       I'd prefer not to add them in the configs as the languages are enumerable and there seems to be an easy way to transform between each. You may create an util function on both backend and frontend to consolidate the format for each library.
   
   




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

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

Posted by GitBox <gi...@apache.org>.
junlincc commented on pull request #13020:
URL: https://github.com/apache/superset/pull/13020#issuecomment-775729813


   @cdmikechen thanks for your first contribution! 
   
   Could you be explicit about what you have changed in this PR?  in Chinese is fine.... 😁 we have plan for addressing this pain point at the system level, but would like to understand your reasoning of this change. 


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

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

Posted by 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

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

Posted by GitBox <gi...@apache.org>.
cdmikechen commented on a change in pull request #13020:
URL: https://github.com/apache/superset/pull/13020#discussion_r572926571



##########
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:
       @ktmud  
   `humanize` and `moment.js` and `superset` use different locale keys. I didn't add variables by myself, so I only get variables in the current `LANGUAGES` config.
   Do we need to expand the current `LANGUAGES` config? Just like this:
   ```python
   LANGUAGES = {
       "en": {"flag": "us", "name": "English", "moment_locale": "en", "humanize_locale": "en", "d3_time_locale": "en-US"},
       "it": {"flag": "it", "name": "Italian", "moment_locale": "it", "humanize_locale": "en", "d3_time_locale": "it-IT"},
       "fr": {"flag": "fr", "name": "French", "moment_locale": "fr", "humanize_locale": "fr_FR", "d3_time_locale": "fr-FR"},
       "zh": {"flag": "cn", "name": "Chinese", "moment_locale": "zh-cn", "humanize_locale": "zh_CN", "d3_time_locale": "zh-CN"},
       "ja": {"flag": "jp", "name": "Japanese", "moment_locale": "ja", "humanize_locale": "jp", "d3_time_locale": "ja-JP"},
       "de": {"flag": "de", "name": "German", "moment_locale": "de", "humanize_locale": "de_DE", "d3_time_locale": "de-DE"},
       "pt": {"flag": "pt", "name": "Portuguese", "moment_locale": "pt", "humanize_locale": "en", "d3_time_locale": "en-US"},
       "pt_BR": {"flag": "br", "name": "Brazilian Portuguese", "moment_locale": "pt-br", "humanize_locale": "pt_BR", "d3_time_locale": "pt-BR"},
       "ru": {"flag": "ru", "name": "Russian", "moment_locale": "ru", "humanize_locale": "ru_RU", "d3_time_locale": "ru-RU"},
       "ko": {"flag": "kr", "name": "Korean", "moment_locale": "ko", "humanize_locale": "ko_KR", "d3_time_locale": "ko-KR"},
   }
   ```
   




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

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

Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on a change in pull request #13020:
URL: https://github.com/apache/superset/pull/13020#discussion_r573386638



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

Review comment:
       @cdmikechen 
   十分感谢您的代码贡献. 如果我们的目标是变更 REST 返回的时间间隔. 看起来我们只需要在messages.po里面加入一些翻译内容就可以.
   ```
   "%s from now"
   "%s ago"
   "a moment"
    "now"
   ```
   您可以参考 `humanize` 的代码
   https://github.com/jmoiron/humanize/blob/2d4143e0aecb719a376dc757b439fc7e41b0d773/src/humanize/time.py#L190-L223
   




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

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

Posted by GitBox <gi...@apache.org>.
cdmikechen commented on a change in pull request #13020:
URL: https://github.com/apache/superset/pull/13020#discussion_r572919482



##########
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 once added the code to change the locale after flask was initialized, but it didn't seem to take effect at that time. I'll do another verification later to see if I remember wrong.




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

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

Posted by GitBox <gi...@apache.org>.
cdmikechen commented on a change in pull request #13020:
URL: https://github.com/apache/superset/pull/13020#discussion_r572917074



##########
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:
       @ktmud
   I've searched in superset, there is only one place where humanize is used, so that I think we may need not call the activate method.
   I've tested most pages and found no problems (including switching languages).




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

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

Posted by GitBox <gi...@apache.org>.
cdmikechen commented on pull request #13020:
URL: https://github.com/apache/superset/pull/13020#issuecomment-775761608


   @junlincc 
   superset 目前部分接口返回的时间格式字段没有根据配置的locale信息进行重新格式化，比如我使用的locale配置的是zh的话，返回的还是默认的英文格式的时间。
   比如在图表列表页面，就会有这样的结果：
   ![chartslist_old](https://user-images.githubusercontent.com/12069428/107336095-b323d800-6af3-11eb-8458-6298694a01ab.jpg)
   


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

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

Posted by GitBox <gi...@apache.org>.
cdmikechen commented on a change in pull request #13020:
URL: https://github.com/apache/superset/pull/13020#discussion_r572906856



##########
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:
       @ktmud 
   Thank you for reviewing my code!
   
   It's used in many places on `superset-front` to format time. The required values in current `LANGUAGES` config and `moment.js`  are inconsistent, and I think using `flag` as a supplement should be  ok with `moment.js`. 
   `LANGUAGES` config :
   ```python
   LANGUAGES = {
       "en": {"flag": "us", "name": "English"},
       "es": {"flag": "es", "name": "Spanish"},
       "it": {"flag": "it", "name": "Italian"},
       "fr": {"flag": "fr", "name": "French"},
       "zh": {"flag": "cn", "name": "Chinese"},
       "ja": {"flag": "jp", "name": "Japanese"},
       "de": {"flag": "de", "name": "German"},
       "pt": {"flag": "pt", "name": "Portuguese"},
       "pt_BR": {"flag": "br", "name": "Brazilian Portuguese"},
       "ru": {"flag": "ru", "name": "Russian"},
       "ko": {"flag": "kr", "name": "Korean"},
   }
   ```
   moment.js locale:
   https://github.com/moment/moment/tree/develop/locale
   
   I  wanted to add a `moment_locale` attribute in `LANGUAGES` before, but I don't think it's necessary for the time being, but I think the existing value is enough, so I adjusted the existing value.
   




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

[GitHub] [superset] cdmikechen edited a comment on pull request #13020: fix: Let superset configure the correct time display according to locale

Posted by GitBox <gi...@apache.org>.
cdmikechen edited a comment on pull request #13020:
URL: https://github.com/apache/superset/pull/13020#issuecomment-775761608


   @junlincc 
   你好~
   superset 目前部分接口返回的时间格式字段没有根据配置的locale信息进行重新格式化，比如我使用的locale配置的是zh的话，返回的还是默认的英文格式的时间。
   比如在图表列表页面，就会有这样的结果：
   ![chartslist_old](https://user-images.githubusercontent.com/12069428/107336095-b323d800-6af3-11eb-8458-6298694a01ab.jpg)
   


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

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

Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #13020:
URL: https://github.com/apache/superset/pull/13020#discussion_r573142441



##########
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:
       Users can set `LANGUAGES` with any locale or custom flags, but locales supported by `moment` is fixed. I think it's easier if we have one Superset `locale` config value everywhere and only do the transformation for each library when calling that library. The supported locales are enumerable so the transformation should be fairly simple.
   
   Either way, I'd recommend limit the scope of this PR to only change humanize for CURD list first. i18n is a complex topic, let's proceed with caution.




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

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

Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #13020:
URL: https://github.com/apache/superset/pull/13020#discussion_r573195993



##########
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:
       It's only used here doesn't mean it will not be used elsewhere in the future. The pollution can happen when someone with another language came in and visit a page that used `humanize` without `activate` being called, they may see the global locale set by another use in another procedure, I.e.
   
   ```
   def func1:
      humanize.activate(...)
   
   def func2:
      no humanize activate
   
   user 1 (locale = zh) -> func1 -> zh
   user 2 (local = fr) -> func2 -> zh
   ```
   
   So it's safer if you always deactivate.




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

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

Posted by GitBox <gi...@apache.org>.
garyganyang commented on pull request #13020:
URL: https://github.com/apache/superset/pull/13020#issuecomment-833168391


   i really want this feature


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

[GitHub] [superset] junlincc edited a comment on pull request #13020: fix: Let superset configure the correct time display according to locale

Posted by GitBox <gi...@apache.org>.
junlincc edited a comment on pull request #13020:
URL: https://github.com/apache/superset/pull/13020#issuecomment-776193989


   thanks @cdmikechen for explaining the use case. 
   
   @ktmud please help me understand the proposed solution from product standpoint - time setting triggered by switching languages in the system? 


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

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

Posted by GitBox <gi...@apache.org>.
junlincc commented on pull request #13020:
URL: https://github.com/apache/superset/pull/13020#issuecomment-776193989


   thanks @cdmikechen for explaining the use case. 
   
   @ktmud please help me understand the proposed solution - time setting triggered by switching languages in the system? 


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

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

Posted by GitBox <gi...@apache.org>.
ktmud commented on pull request #13020:
URL: https://github.com/apache/superset/pull/13020#issuecomment-776199237


   @junlincc Languages are normally tied to a localization setting (time format, number format (e.g., , vs . for decimals)), but not always, as a language can be used in multiple countries/regions.
   
   See: https://www.w3.org/International/questions/qa-i18n


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

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

Posted by GitBox <gi...@apache.org>.
cdmikechen commented on a change in pull request #13020:
URL: https://github.com/apache/superset/pull/13020#discussion_r572929001



##########
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:
       Oh~ I forgot to mention that there is actually a separate locale definition value for time formatting in `d3-format`, I've appended each value.




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

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

Posted by GitBox <gi...@apache.org>.
cdmikechen commented on a change in pull request #13020:
URL: https://github.com/apache/superset/pull/13020#discussion_r572683847



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

Review comment:
       @zhaoyongjie 
   Hi~ My modification is to let superset use the correct language to describe time, such as `7 days ago` -> `7天之前`. 
   Time zone should be an extra treatment. My modification does not involve time zone.




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

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

Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on a change in pull request #13020:
URL: https://github.com/apache/superset/pull/13020#discussion_r573386638



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

Review comment:
       @cdmikechen 
   十分感谢您的代码贡献. 如果我们的目标是变更 REST 返回的时间间隔. 看起来我们只需要在messages.po里面加入一些翻译内容就可以.
   ```
   "%s from now"
   "%s ago"
   "a moment"
    "now"
   ```
   您可以参考 `humanize` 的代码
   https://github.com/jmoiron/humanize/blob/2d4143e0aecb719a376dc757b439fc7e41b0d773/src/humanize/time.py#L190-L223
   




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

[GitHub] [superset] junlincc edited a comment on pull request #13020: fix: Let superset configure the correct time display according to locale

Posted by GitBox <gi...@apache.org>.
junlincc edited a comment on pull request #13020:
URL: https://github.com/apache/superset/pull/13020#issuecomment-775729813


   @cdmikechen thanks for your first contribution! 
   
   Could you be explicit about what you have changed in this PR?  in Chinese is fine.... 🙂


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

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

Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on a change in pull request #13020:
URL: https://github.com/apache/superset/pull/13020#discussion_r572652460



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

Review comment:
       We need a config that accurately sets the time zone instead of relying on the `flask_babel` language setting. 




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