-
Notifications
You must be signed in to change notification settings - Fork 9.7k
[firebase_crashlytics] Handle case where function isn't in class for stack #1831
Conversation
collinjackson
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Thanks for adding a test. One suggestion, which I'll commit for you.
|
|
||
| return new StackTraceElement(className == null ? "" : className, methodName, fileName, Integer.parseInt(lineNumber)); | ||
| return new StackTraceElement( | ||
| className == null ? "" : className, methodName, fileName, Integer.parseInt(lineNumber)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think className ?? "" would be more readable
| className == null ? "" : className, methodName, fileName, Integer.parseInt(lineNumber)); | |
| className ?? "", methodName, fileName, Integer.parseInt(lineNumber)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, never mind, this is Java not Dart :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless you know something I don't, I don't think that Java has a null coalescing operator yet... (oops, for some reason github didn't show me your second comment until after I wrote this)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I agree that's definitely more readable 😄
…stack (flutter#1831) * Handle case where no class in dart and android (ios is fine)
…stack (flutter#1831) * Handle case where no class in dart and android (ios is fine)
Description
Currently, the stacktrace parser can't handle it when there is a method without a class in dart - it instead throws its own exception and loses the original one!
This PR handles the case where there is no class name.
There is a slight caveat to the fix on Android - firebase reporting uses a StackTraceElement which requires a class (which makes sense in Java since you can't have Class-less methods), so on Android class-less methods are reported as
.<functionName>whereas on iOS they are simply reported as<className>.I don't think this is a big problem as there seem to be other differences between how the stacktraces are reported on Android and iOS anyways.
i.e. iOS:

Android:

Related Issues
Checklist
Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes (
[x]). This will ensure a smooth and quick review process.///).flutter analyze) does not report any problems on my PR.Breaking Change
Does your PR require plugin users to manually update their apps to accommodate your change?