You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "lscheibel (via GitHub)" <gi...@apache.org> on 2024/05/03 08:08:48 UTC
[I] Humanize not localised [superset]
lscheibel opened a new issue, #28331:
URL: https://github.com/apache/superset/issues/28331
### Bug description
As far as I can tell and from what I'm seeing in the frontend, strings send from the backend to the frontend that have been formatted with `humanize` are not localised and will always use english. There's a couple of places where these strings are used directly in the frontend but one of them comes from [`superset/models/helpers.py`](https://github.com/apache/superset/blob/e94360486e3db9488119f805f4c118ba5021754a/superset/models/helpers.py#L546) which is then used in the chart editor in [`superset-frontend/src/explore/components/ExploreChartHeader/index.jsx`](https://github.com/apache/superset/blob/e94360486e3db9488119f805f4c118ba5021754a/superset-frontend/src/explore/components/ExploreChartHeader/index.jsx#L201).
### How to reproduce the bug
1. Change the superset language to something other than english.
2. Edit an existing chart.
3. Take a look at the header and the last-modified label.
### Screenshots/recordings
### Superset version
master / latest-dev
### Python version
3.9
### Node version
18 or greater
### Browser
Chrome
### Additional context
I found [flask-humanize](https://github.com/vitalk/flask-humanize) which could be a solution. Although a question would be, how much Superset wants to rely on server-side localised strings. Especially considering that moment.js is already bundled which can achieve similar things in the frontend.
For a PR would you rather see something that fixes the localisation problem in the backend or changes that would send the date string to the frontend, where they would be pretty-printed using moment?
### Checklist
- [X] I have searched Superset docs and Slack and didn't find a solution to my problem.
- [X] I have searched the GitHub issue tracker and didn't find a similar bug report.
- [ ] I have checked Superset's logs for errors and if I found a relevant Python stacktrace, I included it here as text in the "additional context" section.
--
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: notifications-unsubscribe@superset.apache.org.apache.org
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
Re: [I] Humanize not localised [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on issue #28331:
URL: https://github.com/apache/superset/issues/28331#issuecomment-2099394936
It'd be good to dig deeper, but while moment is used in many places in the codebase (looks like about 30 files), I don't think we use much of the api surface there. I also remember fighting with it around the fact that it was bloating our bundles quite a bit. The fact it isn't maintained is always a risk too. Given all this it seems we should replace as opposed to putting more work on top of it.
--
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: notifications-unsubscribe@superset.apache.org
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
Re: [I] Humanize not localised [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on issue #28331:
URL: https://github.com/apache/superset/issues/28331#issuecomment-2093379254
Personally, I think doing this in the front end makes more sense, but it's probably debatable! That would make it easier to leverage in the myriad places we'll want to do so going forward.
One thing that's awkward here is the fact that `moment.js` is no longer maintained. At _some point_ we might want to migrate to `date-fns` or whatever's cool/stable. To minimize the pain here, it might make sense to centralize the logic here as either a utility function in the codebase utils (`get_humanized_time({datetime, lang})` or perhaps a react component that renders a `span` with the appropriate string.
As long as we adhere to the DRY principle and reduce/reuse/recycle logic so it's scalable/maintainable, I'm open to whatever works :) I'll just ping @michael-s-molina @dpgaspar @mistercrunch who might have different/additional opinions on the matter.
Thank you for offering to contribute a PR around this!
--
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: notifications-unsubscribe@superset.apache.org
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
Re: [I] Humanize not localised [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on issue #28331:
URL: https://github.com/apache/superset/issues/28331#issuecomment-2093909694
I think that belongs in the frontend too, ideally backend sends a utc stamp and frontend can show something even relative to `now` that changes with timers and such. What's the new hot library to replace moment?
--
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: notifications-unsubscribe@superset.apache.org
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
Re: [I] Humanize not localised [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on issue #28331:
URL: https://github.com/apache/superset/issues/28331#issuecomment-3180282573
Added some comments there, but it's looking good to me!
--
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: notifications-unsubscribe@superset.apache.org
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
Re: [I] Humanize not localised [superset]
Posted by "hainenber (via GitHub)" <gi...@apache.org>.
hainenber commented on issue #28331:
URL: https://github.com/apache/superset/issues/28331#issuecomment-2094845000
`date-fns` is both stable and cool for some time. It's lightweight, actively maintained and very much feature-parity with `moment.js`. Second vote for that sturdy 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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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
Re: [I] Humanize not localised [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas closed issue #28331: Humanize not localised
URL: https://github.com/apache/superset/issues/28331
--
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: notifications-unsubscribe@superset.apache.org
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
Re: [I] Humanize not localised [superset]
Posted by "ceenayekta (via GitHub)" <gi...@apache.org>.
ceenayekta commented on issue #28331:
URL: https://github.com/apache/superset/issues/28331#issuecomment-2781395214
@michael-s-molina @rusackas can't we add locale to the humanize library used in `superset/models/helpers.py`?
https://github.com/python-humanize/humanize?tab=readme-ov-file#localization
--
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: notifications-unsubscribe@superset.apache.org
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
Re: [I] Humanize not localised [superset]
Posted by "lscheibel (via GitHub)" <gi...@apache.org>.
lscheibel commented on issue #28331:
URL: https://github.com/apache/superset/issues/28331#issuecomment-2097641757
Hm, I see moment.js being in maintaince mode as a separate Problem from what I described initially. I definitely see your arguments regarding phasing out moment but as of today, it is pretty deeply rooted within the frontend. Moment is a dependency not just for antd4 but also the echarts- and handlebars-plugins, as well as a few other legacy plugins.
Therefore I would stick with moment for now, instead of introducing yet another dependency.
I hope to submit a draft PR this week.
--
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: notifications-unsubscribe@superset.apache.org
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
Re: [I] Humanize not localised [superset]
Posted by "ceenayekta (via GitHub)" <gi...@apache.org>.
ceenayekta commented on issue #28331:
URL: https://github.com/apache/superset/issues/28331#issuecomment-2816715185
@michael-s-molina @rusackas here what I came up to: #33188
ps: feel free to give me hints to make it better, I'm not a py developer.
--
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: notifications-unsubscribe@superset.apache.org
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
Re: [I] Humanize not localised [superset]
Posted by "ceenayekta (via GitHub)" <gi...@apache.org>.
ceenayekta commented on issue #28331:
URL: https://github.com/apache/superset/issues/28331#issuecomment-2816724595
@michael-s-molina @rusackas @lscheibel
Please review #33189 and feel free to give comments (cause I'm not a py dev).
--
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: notifications-unsubscribe@superset.apache.org
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
Re: [I] Humanize not localised [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on issue #28331:
URL: https://github.com/apache/superset/issues/28331#issuecomment-2737748186
Can anyone provide an update as to whether this is still an issue that should be kept open in the 4.1.1/5.0.0rc1 world? I was tempted to close this as stale, but don't want to sweep it under the rug if it's still a problem, or anyone's still willing to contribute a fix :)
--
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: notifications-unsubscribe@superset.apache.org
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