You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "villebro (via GitHub)" <gi...@apache.org> on 2023/06/20 11:16:49 UTC
[GitHub] [superset] villebro opened a new pull request, #24457: [WIP] fix(gaq): support localized error messages
villebro opened a new pull request, #24457:
URL: https://github.com/apache/superset/pull/24457
### SUMMARY
Use the user's locale to correctly handle `flask-babel` translated strings, both regular `gettext` and `lazy_gettext`.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] 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.
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: [PR] [WIP] fix(gaq): support localized error messages [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #24457:
URL: https://github.com/apache/superset/pull/24457#issuecomment-2297570848
Convering this to draft since it's WIP and in need of a rebase.
--
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
[GitHub] [superset] giftig commented on a diff in pull request #24457: [WIP] fix(gaq): support localized error messages
Posted by "giftig (via GitHub)" <gi...@apache.org>.
giftig commented on code in PR #24457:
URL: https://github.com/apache/superset/pull/24457#discussion_r1236676649
##########
superset/tasks/async_queries.py:
##########
@@ -96,15 +101,13 @@ def load_chart_data_into_cache(
else str(ex)
)
errors = [{"message": error}]
Review Comment:
Ah I didn't spot the import change to use `flask.json`. So flask's json implementation most likely will be able to deal with `LazyString`, actually.
--
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
[GitHub] [superset] villebro commented on a diff in pull request #24457: [WIP] fix(gaq): support localized error messages
Posted by "villebro (via GitHub)" <gi...@apache.org>.
villebro commented on code in PR #24457:
URL: https://github.com/apache/superset/pull/24457#discussion_r1236680233
##########
superset/tasks/async_queries.py:
##########
@@ -96,15 +101,13 @@ def load_chart_data_into_cache(
else str(ex)
)
errors = [{"message": error}]
Review Comment:
It does. However, similar logic may exist elsewhere, so we may run into this same issue again. So at any rate, we should ensure prudent use of `gettext`/`lazy_gettext` everywhere.
--
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
[GitHub] [superset] giftig commented on a diff in pull request #24457: [WIP] fix(gaq): support localized error messages
Posted by "giftig (via GitHub)" <gi...@apache.org>.
giftig commented on code in PR #24457:
URL: https://github.com/apache/superset/pull/24457#discussion_r1235333260
##########
superset/tasks/async_queries.py:
##########
@@ -96,15 +101,13 @@ def load_chart_data_into_cache(
else str(ex)
)
errors = [{"message": error}]
Review Comment:
I wonder if this code is repeated; feels like I've seen errors reported as `{"message": "(some exception message)"}` fairly frequently so maybe it could be more DRY and be made safe against lazy messaging for good by reusing any logic we need surrounding that.
Presumably even with locale here, we still need to be using `gettext` instead of `gettext_lazy` when raising the error we've caught here; having the locale context will let us translate the message correctly but won't fix the fact that the `json` library doesn't know how to serialise `LazyString`.
--
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
[GitHub] [superset] giftig commented on a diff in pull request #24457: [WIP] fix(gaq): support localized error messages
Posted by "giftig (via GitHub)" <gi...@apache.org>.
giftig commented on code in PR #24457:
URL: https://github.com/apache/superset/pull/24457#discussion_r1236676649
##########
superset/tasks/async_queries.py:
##########
@@ -96,15 +101,13 @@ def load_chart_data_into_cache(
else str(ex)
)
errors = [{"message": error}]
Review Comment:
Ah I didn't spot the import change leading to use `flask.json`. So flask's json implementation most likely will be able to deal with `LazyString`, actually.
--
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: [PR] [WIP] fix(gaq): support localized error messages [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas closed pull request #24457: [WIP] fix(gaq): support localized error messages
URL: https://github.com/apache/superset/pull/24457
--
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
[GitHub] [superset] villebro commented on a diff in pull request #24457: [WIP] fix(gaq): support localized error messages
Posted by "villebro (via GitHub)" <gi...@apache.org>.
villebro commented on code in PR #24457:
URL: https://github.com/apache/superset/pull/24457#discussion_r1236648884
##########
superset/tasks/async_queries.py:
##########
@@ -96,15 +101,13 @@ def load_chart_data_into_cache(
else str(ex)
)
errors = [{"message": error}]
Review Comment:
Good idea. I'm still contemplating different solutions to this problem, as there's multiple things that are wrong right now:
1. The absence of a locale in celery tasks (which this is primarily trying to address)
2. Incorrect use of `lazy_gettext` where `gettext` is sufficient
3. serializing/deserializing using `json`.
Thinking more about this I'm not so sure anymore if we should really be using `flask.json` as I've done here (bullet nr 3). So maybe that part should be reverted, and also that error message logic DRY'd up like you are proposing.
--
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: [PR] [WIP] fix(gaq): support localized error messages [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #24457:
URL: https://github.com/apache/superset/pull/24457#issuecomment-2702154745
This PR hasn't seen any activity/events in roughly seven months. I'll reluctantly close it, but would love to see it reopened/revisited if anyone here has interest or bandwidth.
--
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