[fix] Small fix on Translations.get #564
Conversation
|
@DrBu7cher Thanks for this PR, a very good idea! Maybe we should add a dedicated warning-log in this case, so the user knows what's wrong? Anyways, it would be important to have a unittest covering this, so it's not destroyed by a another change. |
| /// If we failed to find the key as a nested key, then fall back | ||
| /// to looking it up like normal. | ||
| returnValue ??= _translations?[key]; | ||
| var maybeValue = _translations?[key]; |
There was a problem hiding this comment.
Now we do the lookup all the time, even if it's not needed because returnValue != null
| /// to looking it up like normal. | ||
| returnValue ??= _translations?[key]; | ||
| var maybeValue = _translations?[key]; | ||
| if (maybeValue is String) returnValue ??= maybeValue; |
There was a problem hiding this comment.
we need a unittest for the new behavior
jonasbadstuebner
force-pushed
the
develop
branch
from
2cf1f0e to
041eebe
Compare
jonasbadstuebner
force-pushed
the
develop
branch
from
041eebe to
bcf8f05
Compare
|
Good ideas, I will come back to that. |
jonasbadstuebner
force-pushed
the
develop
branch
from
bcf8f05 to
c6da10b
Compare
WalkthroughThe Translations.get method now guards the top-level fallback lookup: it retrieves _translations?[key] into a temporary and assigns it only if the value is a String. Nested-key lookup and caching behavior are unchanged. No public API signatures were modified. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Caller
participant T as Translations.get
participant M as _translations
C->>T: get(key)
alt key contains dot (nested)
T->>T: resolve nested value
alt found String
T-->>C: return value
else not found
T->>M: maybeValue = _translations?[key]
alt maybeValue is String
T-->>C: return maybeValue
else not String / null
T-->>C: return null
end
end
else non-nested
T->>M: maybeValue = _translations?[key]
alt maybeValue is String
T-->>C: return maybeValue
else not String / null
T-->>C: return null
end
end
note over T: Caching behavior unchanged
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/src/translations.dart (1)
36-43: Prevent runtime type errors: only cache/return Strings from nested lookup.Currently,
cacheNestedKey(String value)can receive a non-String (Map/num/bool), causing a runtime type error. Returnnullfor non-String leaves to honorString?API and cache only valid Strings.- /// If we found the value, cache it. If the value is null then - /// we're not going to cache it, and returning null instead. - if (value != null) { - cacheNestedKey(key, value); - } - - return value; + /// Cache and return only String values; treat non-String leaves as not found. + if (value is String) { + cacheNestedKey(key, value); + return value; + } + return null;
♻️ Duplicate comments (2)
lib/src/translations.dart (2)
16-17: Avoid unnecessary map lookup when nested lookup already succeeded.Do the top-level lookup only if
returnValue == null. This addresses bw-flagship’s note while keeping the new type guard.- var maybeValue = _translations?[key]; - if (maybeValue is String) returnValue ??= maybeValue; + if (returnValue == null) { + final v = _translations?[key]; + if (v is String) { + returnValue = v; + } + }
16-17: Add unit tests for the new behavior (top-level non-String and nested non-String).Covers the regression mentioned by bw-flagship and this PR’s fix: returning
null(plus warning) instead of throwing on maps; also covers nested non-String leaves.Proposed test (new file:
test/translations_get_test.dart):import 'package:test/test.dart'; import 'package:easy_localization/src/translations.dart'; void main() { group('Translations.get', () { test('returns null (and warns) when top-level value is a map', () { final t = Translations({ 'error': { 'unknown': 'An unknown error occurred', 'unauthenticated': 'You need to log in first', }, 'literal': 'ok', }); expect(t.get('error'), isNull); // non-leaf key expect(t.get('literal'), 'ok'); expect(t.get('error.unknown'), 'An unknown error occurred'); }); test('nested non-String returns null and is not cached', () { final t = Translations({ 'a': { 'b': {}, // non-leaf at the end }, }); expect(t.get('a.b'), isNull); expect(t.get('a.b.c'), isNull); // should not crash or return a non-String }); }); }
🧹 Nitpick comments (3)
lib/src/translations.dart (3)
16-19: Add a targeted warning when the top-level value is a non-String (object).Users get a clearer hint that they referenced a non-leaf key (e.g., "error" vs "error.unknown"). Keep this behind the same conditional to avoid extra lookups.
- if (returnValue == null) { - final v = _translations?[key]; - if (v is String) { - returnValue = v; - } - } + if (returnValue == null) { + final v = _translations?[key]; + if (v is String) { + returnValue = v; + } else if (v != null) { + // Consider using the project's logger instead of print if available. + print('[🌎 Easy Localization] [WARNING] Localization key [$key] points to an object; use a dotted path like "$key.<leaf>".'); + } + }
32-34: Minor: stop traversing once the path can’t continue.Short-circuit the loop when
valueis no longer a map. Saves pointless iterations and clarifies intent.- for (var i = 1; i < keys.length; i++) { - if (value is Map<String, dynamic>) value = value[keys[i]]; - } + for (var i = 1; i < keys.length && value is Map<String, dynamic>; i++) { + value = (value as Map<String, dynamic>)[keys[i]]; + }
59-60: Null-safety: avoid!on_translationsinisNestedKey.Prevents a possible NPE if
_translationsis null.- bool isNestedKey(String key) => - !_translations!.containsKey(key) && key.contains('.'); + bool isNestedKey(String key) => + (_translations != null && !_translations!.containsKey(key)) && key.contains('.');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
lib/src/translations.dart(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
2 participants
I got an error like this:
from
Translations.get.I had a JSON for translation like
{ "error": { "unknown": "An unknown error occurred", "unauthenticated": "You need to log in first" } }etc.
My code was trying to translate like
tr(errorString)whereerrorStringwas"error"and this led to the lookup in lib/src/translations.dart#L16 to try and put the resulting Map ({ "unknown": "An unknown error occurred", "unauthenticated": "You need to log in first" }) into a String?.
This PR fixes the problem and just gives
[🌎 Easy Localization] [WARNING] Localization key [error] not foundas expected.I did not check in any other places, if this could happen. If an error occurs somewhere else, I will open a new PR.
By the way, when will you release the next time? The last release on pub.dev was 10 months ago...?
Summary by CodeRabbit