-
Notifications
You must be signed in to change notification settings - Fork 63
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
Catch more generic exceptions for InstallReferrer (close #647) #681
Conversation
snowplow-tracker/src/main/java/com/snowplowanalytics/core/tracker/InstallReferrerDetails.kt
Outdated
Show resolved
Hide resolved
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.
LGTM
As the code will always crash without the dependency, why was the decision to catch Exception instead of updating transitive dependencies?. Can this PR also update about the missing info in the doc |
Hi @VenomVendor good shout about the docs! I don't understand the first part of your message sorry. The code already caught when the dependency is completely absent, now it will also catch versions without the required functionality. |
I meant, why to catch the exception if we can update transitive dependency. I was in an assumption that the dependency install-referrer is mandatory. Found this check |
For issue #647
The
InstallReferrerDetails
class currently checks whether the InstallReferrer dependency is present or not, but if it is, then only catchesRemoteException
s. InstallReferrer below v1.1 doesn't have thegooglePlayInstantParam
property which causes aNoSuchMethodError
.Unfortunately the
googlePlayInstantParam
is the one required property for thereferrer_details
schema, so if InstallReferrer v1.0 is used it's not possible to add the entity at all.This change catches the
NoSuchMethodError
.