Formatted lines with 80 characters, null crash fix, English fixes #144

noordawod · 2021-01-01T12:15:11Z

In a production app I used get_it and I had a crash with the nullsafety branch. Turns out that isRegistered() calls _findFactoryByNameAndType() which throws an exception because the returned value is null, but is silenced using the bang (!) character.

So I fixed that and also reformatted the source code to 100 characters per line (it was not formatted it seemed). Also, many English grammatical and pronunciation errors were fixed.

…ed().

noordawod · 2021-01-01T12:18:01Z

lib/get_it_impl.dart

+      ' type ${T.toString()} is not registered inside GetIt. '
+      '\n(Did you accidentally do GetIt sl=GetIt.instance(); instead of GetIt sl=GetIt.instance;'
+      '\nDid you forget to register it?)',
+    );

    return instanceFactory!;


This line caused a hard crash in a production app because assert() did not run. In general, use of ! is discouraged and one should find an alternative to yield the eventual logic.

In this case, for example, one can use throwIf() which is guaranteed to run in production.

I disagree, if you your crash wasn't caught during debug something is wrong how you test your code and normally you always have some gloabl execption handler that logs such cases.
Also you didn't change the assert. How did you fix the problem?

noordawod · 2021-01-06T23:02:46Z

Anybody here? @escamoteur?

escamoteur · 2021-01-07T07:11:48Z

Hi, sorry I forgot to merge you. Will do it today Am 07.01.2021 00:02 schrieb Noor Dawod <[email protected]>: Anybody here? @escamoteur? —You are receiving this because you were mentioned.Reply to this email directly, view it on GitHub, or unsubscribe.

noordawod · 2021-01-07T16:22:25Z

Reminder @escamoteur

escamoteur

Hi,
thanks for your PR.
My problem is, that it was intentional that the line length was set to 80, because pub's rating system punishes you if you use another length. Don't know if they have changed it. As this is a prerelease we probably can try.
Please see my other comments.
cheers
Thomas

escamoteur · 2021-01-07T22:52:29Z

.editorconfig

@@ -0,0 +1,10 @@
+root = true


fot which editor it this config file?

https://editorconfig.org/

I see you're working in VSCode. I work in IntelliJ IDEA. The beauty of .editorconfig is that it's cross-IDE and most IDEs out there can understand it and follow its configuration.

example/lib/main.dart

escamoteur · 2021-01-07T23:02:58Z

lib/get_it_impl.dart

+      ' type ${T.toString()} is not registered inside GetIt. '
+      '\n(Did you accidentally do GetIt sl=GetIt.instance(); instead of GetIt sl=GetIt.instance;'
+      '\nDid you forget to register it?)',
+    );

    return instanceFactory!;


I disagree, if you your crash wasn't caught during debug something is wrong how you test your code and normally you always have some gloabl execption handler that logs such cases.
Also you didn't change the assert. How did you fix the problem?

noordawod · 2021-01-07T23:18:37Z

I can reformat with 80 characters, no worries.

Regarding the crash: If you notice I added a couple of new methods and then used them in isRegistered() in order NOT to crash the app.

  /// Tests if an [instance] of an object or aType [T] or a name [instanceName]
  /// is registered inside GetIt
  @override
  bool isRegistered<T extends Object>({
    Object? instance,
    String? instanceName,
  }) {
    if (instance != null) {
      return _findFirstFactoryByInstanceOrNull(instance) != null;
    } else {
      return _findFirstFactoryByNameAndTypeOrNull<T>(instanceName) != null;
    }
  }

Today, isRegistered() calls _findFactoryByNameAndType() which returns a non-null value (that's why I didn't change it). But that causes isRegistered() to crash if that dependency isn't registered yet.

noordawod · 2021-01-07T23:38:31Z

@escamoteur reformatted the code to be 80 characters using flutter format.

escamoteur · 2021-01-08T11:46:40Z

Hi,

please never do a reformating and a shcnage in the same PR. I couldn't spot your changes because the diff is all red because of the reformatting.
Why did you add new methods instead of just use an throwIf isntead of the assert?
I'm trying to keep the null save version as close to the last official one as possible.

noordawod · 2021-01-08T18:35:11Z

Forget it. This is the most inhospitable repo on the planet, I'm changing DI.

escamoteur · 2021-01-08T18:51:48Z

Sorry if you call it inhospital to ask reasonable question.
do what yyou want.

noordawod · 2021-01-08T19:11:10Z

Why did you add new methods instead of just use an throwIf isntead of the assert?

I explained twice already. Re-read.

escamoteur · 2021-01-08T23:04:18Z

I just reread the whole converstion and you didn't. You proposed at the beginning we could use ´throwIf´but then you went for another solution without any explanation.

noordawod · 2021-01-08T23:18:47Z

3rd time is charm maybe... repeating:

Regarding the crash: If you notice I added a couple of new methods and then used them in isRegistered() in order NOT to crash the app.

  /// Tests if an [instance] of an object or aType [T] or a name [instanceName]
  /// is registered inside GetIt
  @override
  bool isRegistered<T extends Object>({
    Object? instance,
    String? instanceName,
  }) {
    if (instance != null) {
      return _findFirstFactoryByInstanceOrNull(instance) != null;
    } else {
      return _findFirstFactoryByNameAndTypeOrNull<T>(instanceName) != null;
    }
  }

Today, isRegistered() calls _findFactoryByNameAndType() which returns a non-null value (that's why I didn't change it). But that causes isRegistered() to crash if that dependency isn't registered yet.

I'll simplify more:

Current code in PROD:

The app starts and T isn't registered yet.
You call isRegistered<T>()
isRegistered() delegates to _findFactoryByNameAndType<T>()
_findFactoryByNameAndType() crashes because T is not registered yet.
The crash happens in PROD because assert() never runs, and the method ends with return instanceFactory! which is an Error – instanceFactory is not registered yet so it's null.

Proposed fix:

The app starts and T isn't registered yet.
You call isRegistered<T>()
isRegistered() delegates either to _findFirstFactoryByInstanceOrNull<T>() or _findFirstFactoryByNameAndTypeOrNull<T>().
These two methods are new in the proposed fix, they do not crash if T isn't registered.
isRegistered() returns the correct result, without crashing.

I hope this clears things up now?

escamoteur · 2021-01-08T23:58:02Z

I totally got that. My question was why not just change the assert to a ´throwIf´ then isRegistered would work as expected?

noordawod · 2021-01-09T11:02:26Z

I totally got that. My question was why not just change the assert to a ´throwIf´ then isRegistered would work as expected?

I believe it would work because isRegistered has a try-catch clause. It's in line with how the code is currently written, so I'm fine with patching it. This will also keep everything else intact, just how you like it ;)

Closing this pr.

noordawod · 2021-01-09T11:03:42Z

ps: there were some English grammatical fixes but you can do that at your leisure.

escamoteur · 2021-01-09T12:35:30Z

Would love to only how to filter them out of the formatting?

noordawod · 2021-01-10T15:04:09Z

Glad this got merged! I think it was a good call.

escamoteur · 2021-01-10T15:37:43Z

Did you run the tests after the changes? Because here they failed after the merge.

You did forget to pass T from _findFactoryByTypeAndName to your _findFirstFactoryOrNull

escamoteur · 2021-01-10T15:52:20Z

Thanks again for your effort. After I copied the latest version from your PR into a new branch locally I could easier follow what you had changed.

noordawod · 2021-01-10T16:28:03Z

Right, good! Will you fix the issue with passing T?

escamoteur · 2021-01-10T19:14:47Z

Already done and new versions on pub

noordawod · 2021-01-11T09:01:18Z

Thanks @escamoteur, this has been a good experience :)

escamoteur · 2021-01-11T11:39:18Z

I accept this as an apology ;-)

noordawod · 2021-01-11T17:26:31Z

Might as well :)

noordawod added 4 commits January 1, 2021 12:31

Add IDE-friendly editor config file.

2d4937c

Set maximum line length to 100 characters.

4d4013c

Reformatted code, fixed grammatical errors, fix a crash in isRegister…

51f3e92

…ed().

Reformatted code with 100 characters per line.

8f2dcdb

noordawod commented Jan 1, 2021

View reviewed changes

noordawod changed the title ~~Bugfixes formatting~~ Formatted lines with 100 characters, null crash fix, English fixes Jan 1, 2021

noordawod added 2 commits January 1, 2021 13:20

Bump up version.

88859b9

Fix double spaces in error message.

75ae080

escamoteur reviewed Jan 7, 2021

View reviewed changes

Reformat the code using "flutter format", set line to 80 characters.

74fbe58

noordawod changed the title ~~Formatted lines with 100 characters, null crash fix, English fixes~~ Formatted lines with 80 characters, null crash fix, English fixes Jan 7, 2021

noordawod closed this Jan 8, 2021

noordawod reopened this Jan 8, 2021

noordawod closed this Jan 9, 2021

escamoteur reopened this Jan 10, 2021

escamoteur merged commit 23b01ac into fluttercommunity:null_safety Jan 10, 2021

noordawod deleted the bugfixes_formatting branch January 10, 2021 15:04

Uh oh!

Formatted lines with 80 characters, null crash fix, English fixes #144

Formatted lines with 80 characters, null crash fix, English fixes #144

Uh oh!

Conversation

noordawod commented Jan 1, 2021

Uh oh!

noordawod Jan 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

escamoteur Jan 7, 2021

Choose a reason for hiding this comment

Uh oh!

noordawod commented Jan 6, 2021

Uh oh!

escamoteur commented Jan 7, 2021 via email

Uh oh!

noordawod commented Jan 7, 2021

Uh oh!

escamoteur left a comment

Choose a reason for hiding this comment

Uh oh!

escamoteur Jan 7, 2021

Choose a reason for hiding this comment

Uh oh!

noordawod Jan 7, 2021

Choose a reason for hiding this comment

Uh oh!

noordawod Jan 7, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

escamoteur Jan 7, 2021

Choose a reason for hiding this comment

Uh oh!

noordawod commented Jan 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

noordawod commented Jan 7, 2021

Uh oh!

escamoteur commented Jan 8, 2021

Uh oh!

noordawod commented Jan 8, 2021

Uh oh!

escamoteur commented Jan 8, 2021

Uh oh!

noordawod commented Jan 8, 2021

Uh oh!

escamoteur commented Jan 8, 2021

Uh oh!

noordawod commented Jan 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

escamoteur commented Jan 8, 2021

Uh oh!

noordawod commented Jan 9, 2021

Uh oh!

noordawod commented Jan 9, 2021

Uh oh!

escamoteur commented Jan 9, 2021

Uh oh!

noordawod commented Jan 10, 2021

Uh oh!

escamoteur commented Jan 10, 2021

Uh oh!

escamoteur commented Jan 10, 2021

Uh oh!

noordawod commented Jan 10, 2021

Uh oh!

escamoteur commented Jan 10, 2021

Uh oh!

noordawod commented Jan 11, 2021

Uh oh!

escamoteur commented Jan 11, 2021

Uh oh!

noordawod commented Jan 11, 2021

Uh oh!

Uh oh!

noordawod Jan 1, 2021 •

edited

Loading

noordawod commented Jan 7, 2021 •

edited

Loading

noordawod commented Jan 8, 2021 •

edited

Loading