[Fixes #13800] Refact metadata i18n mechanism#13801
Conversation
Summary of ChangesHello @etj, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the internationalization (i18n) mechanism for metadata within the GeoNode project. The changes aim to centralize and streamline how localized metadata labels and schemas are managed and accessed. By relocating core i18n components to the 'geonode.base' application, introducing a dedicated 'LabelResolver' for efficient label retrieval, and enhancing the caching strategy, the PR improves modularity, maintainability, and consistency of i18n across the platform. Furthermore, the refactoring includes a cleaner approach to handling database signals related to thesaurus updates, ensuring that i18n caches are invalidated and refreshed appropriately. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
/gemini review |
There was a problem hiding this comment.
Code Review
The pull request refactors the internationalization (i18n) mechanism by moving core logic and signal handling from the metadata application to the base application. This improves modularity and separation of concerns. Key changes include renaming geonode/metadata/i18n.py to geonode/base/i18n.py, introducing a new geonode/base/signals.py for centralized signal connections, and restructuring the caching mechanism with I18nCacheEntry and LabelResolver classes. The changes also involve updating import paths and context keys across various files to reflect the new structure. The introduction of TODO comments is helpful for tracking future improvements.
|
|
||
| def __init__(self): | ||
| # the cache has the lang as key, and various info in the dict value: | ||
| # - date: the date field of the thesaurus when it was last loaded, it's used for the expiration check | ||
| # - labels: the keyword labels from the i18n thesaurus | ||
| # - schema: the localized json schema | ||
| # FIFO bc we want to renew the data once in a while | ||
| self.cache = FIFOCache(16) | ||
| # the cache has the lang as key, and I18nCacheEntry as a value: | ||
| self.lang_cache = {} |
There was a problem hiding this comment.
The replacement of FIFOCache with a standard dictionary (self.lang_cache) removes the fixed-size eviction policy. If the number of languages or cached data keys grows large, this could lead to unbounded memory consumption. Consider if an unbounded cache is the desired behavior, or if a size-limited cache (e.g., using functools.lru_cache or a custom implementation) is more appropriate to prevent potential memory issues in a production environment.
| def __init__(self): | |
| # the cache has the lang as key, and various info in the dict value: | |
| # - date: the date field of the thesaurus when it was last loaded, it's used for the expiration check | |
| # - labels: the keyword labels from the i18n thesaurus | |
| # - schema: the localized json schema | |
| # FIFO bc we want to renew the data once in a while | |
| self.cache = FIFOCache(16) | |
| # the cache has the lang as key, and I18nCacheEntry as a value: | |
| self.lang_cache = {} | |
| def __init__(self): | |
| # the cache has the lang as key, and I18nCacheEntry as a value: | |
| self.lang_cache = {} |
| @@ -72,7 +73,7 @@ def update_resource( | |||
| ): | |||
| """ | |||
| Called when persisting data, updates the field `field_name` of the resource | |||
There was a problem hiding this comment.
There's a small typo in the docstring. "content content" should be "with the content".
| Called when persisting data, updates the field `field_name` of the resource | |
| with the content, where json_instance is the full JSON Schema instance, |
etj
force-pushed
the
13800_i18n_refact
branch
5 times, most recently
from
edcf953 to
31c3aed
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #13801 +/- ##
==========================================
+ Coverage 74.13% 74.14% +0.01%
==========================================
Files 944 944
Lines 56291 56321 +30
Branches 7617 7623 +6
==========================================
+ Hits 41730 41761 +31
+ Misses 12897 12893 -4
- Partials 1664 1667 +3 🚀 New features to boost your workflow:
|
etj
force-pushed
the
13800_i18n_refact
branch
from
31c3aed to
f2e1406
Compare
etj
force-pushed
the
13800_i18n_refact
branch
from
f2e1406 to
e37e65b
Compare
2 participants
Checklist
For all pull requests:
The following are required only for core and extension modules (they are welcomed, but not required, for contrib modules):
Submitting the PR does not require you to check all items, but by the time it gets merged, they should be either satisfied or inapplicable.