Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[v3] migrating to null safety #334

Merged
merged 53 commits into from
Feb 20, 2021
Merged

[v3] migrating to null safety #334

merged 53 commits into from
Feb 20, 2021

Conversation

aissat
Copy link
Owner

@aissat aissat commented Feb 15, 2021

No description provided.

@acoutts
Copy link
Contributor

acoutts commented Feb 17, 2021

Hi there is something wrong on this branch- when a key isn't found, the app hits an exception. In the old lib, if a key wasn't found, it failed silently so it didn't interrupt the app in any unexpected manner.

Unhandled exception:
Exception: Cannot cache a key that is not nested.
#0      Translations.getNested (package:easy_localization/src/translations.dart:24:7)
#1      Translations.get (package:easy_localization/src/translations.dart:7:27)
#2      Localization.Eval ()
#3      Localization._resolve (package:easy_localization/src/localization.dart:171:18)

Seems like isNested() returns true on keys which aren't found.

Ahh, so if the string contains a period it's now confused because it thinks it's a nested string key!

ie: 'Processing...'.tr() doesn't work with the nested stuff.

Can we optionally disable nested checks completely? I don't want to use a foo.bar.baz key setup, i want to simply use english strings within the app and match it exactly to the json keys.

@aissat
Copy link
Owner Author

aissat commented Feb 17, 2021

Hi there is something wrong on this branch- when a key isn't found, the app hits an exception. In the old lib, if a key wasn't found, it failed silently so it didn't interrupt the app in any unexpected manner.

Unhandled exception:
Exception: Cannot cache a key that is not nested.
#0      Translations.getNested (package:easy_localization/src/translations.dart:24:7)
#1      Translations.get (package:easy_localization/src/translations.dart:7:27)
#2      Localization.Eval ()
#3      Localization._resolve (package:easy_localization/src/localization.dart:171:18)

Seems like isNested() returns true on keys which aren't found.

Ahh, so if the string contains a period it's now confused because it thinks it's a nested string key!

ie: 'Processing...'.tr() doesn't work with the nested stuff.

Can we optionally disable nested checks completely? I don't want to use a foo.bar.baz key setup, i want to simply use english strings within the app and match it exactly to the json keys.

hi @acoutts plz more detailed example for this bug

@acoutts
Copy link
Contributor

acoutts commented Feb 17, 2021

Add this in your json file:

{
  "Please tap the button..": "Please tap the button.."
}

Then in your app,

Text('Please tap the button..'.tr())

The key is the lang key should contain a period.

@aissat
Copy link
Owner Author

aissat commented Feb 17, 2021

Add this in your json file:

{
  "Please tap the button..": "Please tap the button.."
}

Then in your app,

Text('Please tap the button..'.tr())

The key is the lang key should contain a period.

@acoutts I write test case for this

      test('instance.tr() ', () {
        expect(Localization.instance.tr('Please tap the button..'), 'Please tap the button..');
      });

      test('tr()', () {
        expect(tr('Please tap the button..'), 'Please tap the button..');
      });

      test('ext tr()', () {
        expect('Please tap the button..'.tr(), 'Please tap the button..');
      });

test passed

image

@acoutts
Copy link
Contributor

acoutts commented Feb 17, 2021

Hm how strange! So how does the nested key logic distinguish between if you are doing foo.bar.baz nested JSON vs Some full sentence... at the root level?

@aissat
Copy link
Owner Author

aissat commented Feb 17, 2021

@acoutts this is some nested test case

'nested.but.not.nested': 'nested but not nested',

'nested': {
'super': {
'duper': {
'nested': 'nested.super.duper.nested',
'nested_with_arg': 'nested.super.duper.nested_with_arg {}',
'nested_with_named_arg':
'nested.super.duper.nested_with_named_arg {arg}'
}
}
},

test('can resolve resource in any nest level', () {
expect(
Localization.instance.tr('nested.super.duper.nested'),
'nested.super.duper.nested',
);
});
test('can resolve resource that has a key with dots', () {
expect(
Localization.instance.tr('nested.but.not.nested'),
'nested but not nested',
);
});

test('returns resource and replaces argument in any nest level', () {
expect(
Localization.instance
.tr('nested.super.duper.nested_with_arg', args: ['what a nest']),
'nested.super.duper.nested_with_arg what a nest',
);
});

@acoutts
Copy link
Contributor

acoutts commented Feb 17, 2021

Ah very cool- thanks for showing! Can you see if having 3 periods breaks it? That's the only difference I found from my app vs the test there.

@aissat
Copy link
Owner Author

aissat commented Feb 17, 2021

@acoutts check this maybe your issue related to #337 #331 or if u can create an example or test case failed

@acoutts
Copy link
Contributor

acoutts commented Feb 17, 2021

Thanks! That looks related to my issue.

One other thing, I tried to disable the log messages for tests:

  EasyLocalization.logger.enableLevels = <LevelMessages>[LevelMessages.error];

But this one still prints every time a test starts:

[🌎 Easy Localization] [DEBUG] Localization initialized

Is there a way to hide that one?

@acoutts
Copy link
Contributor

acoutts commented Feb 19, 2021

Ok I found how to reproduce the issue. The issue is if you try to reference a key that doesn't exist, and that key has a period in it.

      test('won\'t fail for missing key with consecutive dots', () {
        expect(
          Localization.instance.tr('Processing.'),
          'Processing.',
        );
      });
Exception: Cannot cache a key that is not nested.
package:easy_localization/src/translations.dart 24:7    Translations.getNested
package:easy_localization/src/translations.dart 7:27    Translations.get
package:easy_localization/src/localization.dart 171:35  Localization._resolve
package:easy_localization/src/localization.dart 53:13   Localization.tr
test/easy_localization_test.dart 163:33                 main.<fn>.<fn>.<fn>

Problem is the logic here:

  bool isNestedKey(String key) =>
      !_translations!.containsKey(key) && key.contains('.');

@acoutts
Copy link
Contributor

acoutts commented Feb 19, 2021

Yeah this is tough. I don't think you can figure out if the user is using

  • nested keys welcome_screen.password.title
  • sentences Hello. Welcome to the app
  • nested sentences welcome_screen.Welcome to the app
    etc

@acoutts
Copy link
Contributor

acoutts commented Feb 19, 2021

I was able to fix the logic so it won't fail on missing keys with periods, and all of the tests are passing including the shared preferences null error. I had to put SharedPreferences.setMockInitialValues({}); before await EasyLocalization.ensureInitialized();.

I created this PR to put the changes into your branch: #341

All tests are now passing.

@aissat
Copy link
Owner Author

aissat commented Feb 19, 2021

@acoutts nice work 👍

@acoutts
Copy link
Contributor

acoutts commented Feb 19, 2021

Thanks! Love this package, glad I could help.

Yeah this is tough. I don't think you can figure out if the user is using

  • nested keys welcome_screen.password.title
  • sentences Hello. Welcome to the app
  • nested sentences welcome_screen.Welcome to the app
    etc

This is the only head scratcher. I almost think the user needs to give a hint here what kind of key they are using.

@aissat
Copy link
Owner Author

aissat commented Feb 19, 2021

Thanks! Love this package, glad I could help.

Yeah this is tough. I don't think you can figure out if the user is using

  • nested keys welcome_screen.password.title
  • sentences Hello. Welcome to the app
  • nested sentences welcome_screen.Welcome to the app
    etc

This is the only head scratcher. I almost think the user needs to give a hint here what kind of key they are using.

thnx @acoutts , welcome anytime

@aissat aissat merged commit 33aee74 into develop Feb 20, 2021
@aissat aissat deleted the null-safety.3 branch February 20, 2021 12:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants