korbit-ai[bot] commented on code in PR #34051:
URL: https://github.com/apache/superset/pull/34051#discussion_r2182422528
##########
superset/models/helpers.py:
##########
@@ -546,10 +546,28 @@ def changed_on_utc(self) -> str:
@property
def changed_on_humanized(self) -> str:
+ locale = str(get_locale() or 'en')
+ try:
+ humanize.i18n.activate(locale)
+ result = humanize.naturaltime(datetime.now() - self.changed_on)
+ humanize.i18n.deactivate()
+ return result
Review Comment:
### Inefficient Repeated Locale Switching <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Repeated activation/deactivation of locale in humanize module for each
method call causes unnecessary overhead.
###### Why this matters
Each time changed_on_humanized or created_on_humanized is called, it
activates and deactivates the locale. For lists of objects, this creates
significant performance overhead with multiple redundant locale switches.
###### Suggested change ∙ *Feature Preview*
Move locale activation to a context manager or decorator to handle locale
switching once for multiple calls. Example:
```python
@contextmanager
def use_locale(locale_name):
humanize.i18n.activate(locale_name)
try:
yield
finally:
humanize.i18n.deactivate()
def changed_on_humanized(self) -> str:
locale = str(get_locale() or 'en')
with use_locale(locale):
return humanize.naturaltime(datetime.now() - self.changed_on)
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/62cbd696-0510-48df-84eb-776ee76bfc2f/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/62cbd696-0510-48df-84eb-776ee76bfc2f?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/62cbd696-0510-48df-84eb-776ee76bfc2f?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/62cbd696-0510-48df-84eb-776ee76bfc2f?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/62cbd696-0510-48df-84eb-776ee76bfc2f)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:8cd3c11e-94f8-4bcc-b7a1-345e645ae1ed -->
[](8cd3c11e-94f8-4bcc-b7a1-345e645ae1ed)
##########
superset/models/helpers.py:
##########
@@ -546,10 +546,28 @@ def changed_on_utc(self) -> str:
@property
def changed_on_humanized(self) -> str:
+ locale = str(get_locale() or 'en')
Review Comment:
### Hardcoded default locale <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Magic string 'en' hardcoded as fallback locale
###### Why this matters
Hardcoded strings make the code less maintainable and harder to configure
globally.
###### Suggested change ∙ *Feature Preview*
Define a constant at module level:
```python
DEFAULT_LOCALE = "en"
# Then use in code
locale = str(get_locale() or DEFAULT_LOCALE)
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3c9745e8-30ec-4660-ab42-f686587c903a/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3c9745e8-30ec-4660-ab42-f686587c903a?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3c9745e8-30ec-4660-ab42-f686587c903a?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3c9745e8-30ec-4660-ab42-f686587c903a?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3c9745e8-30ec-4660-ab42-f686587c903a)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:152a6cbf-44d6-4016-93d7-5848477ce7f7 -->
[](152a6cbf-44d6-4016-93d7-5848477ce7f7)
##########
superset-frontend/src/preamble.ts:
##########
@@ -26,6 +26,20 @@ import setupFormatters from './setup/setupFormatters';
import setupDashboardComponents from './setup/setupDashboardComponents';
import { User } from './types/bootstrapTypes';
import getBootstrapData, { applicationRoot } from './utils/getBootstrapData';
+import 'dayjs/locale/en';
+import 'dayjs/locale/fr';
+import 'dayjs/locale/es';
+import 'dayjs/locale/it';
+import 'dayjs/locale/zh-cn';
+import 'dayjs/locale/ja';
+import 'dayjs/locale/de';
+import 'dayjs/locale/pt';
+import 'dayjs/locale/pt-br';
+import 'dayjs/locale/ru';
+import 'dayjs/locale/ko';
+import 'dayjs/locale/sk';
+import 'dayjs/locale/sl';
+import 'dayjs/locale/nl';
Review Comment:
### Inefficient Static Locale Imports <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
All locale imports are statically imported, leading to unnecessary bundle
size increase even when locales aren't used.
###### Why this matters
This approach loads all locale data into the bundle regardless of which
locales are actually needed by the application, impacting initial load
performance.
###### Suggested change ∙ *Feature Preview*
Implement dynamic imports for locales based on the user's selected language:
```typescript
async function loadLocale(locale: string) {
await import(`dayjs/locale/${locale}`);
dayjs.locale(locale);
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5e7d7632-0faf-4958-8a0a-d4c3ecd4d162/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5e7d7632-0faf-4958-8a0a-d4c3ecd4d162?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5e7d7632-0faf-4958-8a0a-d4c3ecd4d162?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5e7d7632-0faf-4958-8a0a-d4c3ecd4d162?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5e7d7632-0faf-4958-8a0a-d4c3ecd4d162)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:09a98c94-191f-4e09-850c-0d5fc57c0cd6 -->
[](09a98c94-191f-4e09-850c-0d5fc57c0cd6)
##########
superset/models/helpers.py:
##########
@@ -546,10 +546,28 @@ def changed_on_utc(self) -> str:
@property
def changed_on_humanized(self) -> str:
+ locale = str(get_locale() or 'en')
+ try:
+ humanize.i18n.activate(locale)
+ result = humanize.naturaltime(datetime.now() - self.changed_on)
+ humanize.i18n.deactivate()
+ return result
+ except:
+ pass
+ # Fallback to English
return humanize.naturaltime(datetime.now() - self.changed_on)
@property
def created_on_humanized(self) -> str:
+ locale = str(get_locale() or 'en')
+ try:
+ humanize.i18n.activate(locale)
+ result = humanize.naturaltime(datetime.now() - self.created_on)
+ humanize.i18n.deactivate()
+ return result
+ except:
+ pass
+ # Fallback to English
return humanize.naturaltime(datetime.now() - self.created_on)
Review Comment:
### Redundant Time Formatting Logic <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Duplicate code blocks in changed_on_humanized and created_on_humanized
methods that handle locale-aware time formatting
###### Why this matters
Violates DRY principle leading to maintenance overhead and increased risk of
inconsistencies when code needs to be updated
###### Suggested change ∙ *Feature Preview*
Extract the common time formatting logic into a private helper method:
```python
def _format_time_humanized(self, timestamp: datetime) -> str:
locale = str(get_locale() or 'en')
try:
humanize.i18n.activate(locale)
result = humanize.naturaltime(datetime.now() - timestamp)
humanize.i18n.deactivate()
return result
except:
return humanize.naturaltime(datetime.now() - timestamp)
@property
def changed_on_humanized(self) -> str:
return self._format_time_humanized(self.changed_on)
@property
def created_on_humanized(self) -> str:
return self._format_time_humanized(self.created_on)
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cf0daa08-d93b-485f-9e42-cae7c98a5dab/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cf0daa08-d93b-485f-9e42-cae7c98a5dab?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cf0daa08-d93b-485f-9e42-cae7c98a5dab?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cf0daa08-d93b-485f-9e42-cae7c98a5dab?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cf0daa08-d93b-485f-9e42-cae7c98a5dab)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:cf155ae6-358f-4da8-9777-dcbdef6bf8bd -->
[](cf155ae6-358f-4da8-9777-dcbdef6bf8bd)
--
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]
