-
-
Notifications
You must be signed in to change notification settings - Fork 19
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
feat: override native options #221
Conversation
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #221 +/- ##
==========================================
+ Coverage 77.66% 78.52% +0.85%
==========================================
Files 27 31 +4
Lines 412 419 +7
Branches 56 56
==========================================
+ Hits 320 329 +9
+ Misses 66 64 -2
Partials 26 26 ☔ View full report in Codecov by Sentry. |
...tform/src/androidMain/kotlin/io/sentry/kotlin/multiplatform/SentryPlatformOptions.android.kt
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!
anything against making the native dependencies api
? I think that's what we do on RN for example
@romtsn actually I'd love to add it, then we can freely typealias stuff directly to the android/java sdk and the user doesn't need to add the only thing speaking against it is that you'd have access to the android/java sdk directly in your android sourceSet but not sure if that's really so bad The user just needs to be careful not to confuse |
Let's expose it, I think we can always get back and re-think if there are problems. I also expect problems for those who use multiple sentry SDKs (e.g. RN and KMP which both expose the Android sdk), but then it's kind of not our problem and they'd have to resolve conflicts themselves. Just have to make sure we're not lagging behind with SDK updates for major bumps/breaking changes |
📜 Description
Allows user to init with
initWithPlatformOptions { }
in the respective platform sourceSet which directly calls the native optionsRequirements for being able to use this:
Also open to changes to the API name
(the implementation is a pretty simple addition but changing multiple platforms in KMP just blows up the files changed stats)
💡 Motivation and Context
Implements #213
💚 How did you test it?
Unit tests
📝 Checklist
You have to check all boxes before merging:
sendDefaultPII
is enabled.🔮 Next steps