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

Move LogCat prop to SentryOptions #2937

Closed
wants to merge 7 commits into from
Closed

Conversation

kanadaj
Copy link
Contributor

@kanadaj kanadaj commented Dec 2, 2023

Addresses #2935 (Move LogCat prop to SentryOptions)

Copy link
Member

@bruno-garcia bruno-garcia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a partial class defined in Platform/Android for SentryOptions so the properties can stay there (just not under the native binding options.Android that map to the native SDK options:

main/src/Sentry/Platforms/Android/SentryOptions.cs

All Android specific code that can go there is better located under Platform/Android (similar to a MAUI app structure).
We can avoid #ifdef at the top level stuff a lot this way.

@@ -1,7 +1,8 @@
#if ANDROID
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could stay in Sentry/Platform/Android, no need to move it up.

Copy link
Member

@bruno-garcia bruno-garcia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but looking at the sample made me realize that having it under Android had the advantage of not requiring additional #ifdef from the client app. Since the whole property is by definition Android specific.
The fact it breaks the simple explanation that's "binding the native Android SDK" is the challenge.

So I'm happy making this change, but at the same feel less strongly we must, because I see the trade off of the original approach vs this.

cc @bitsandfoxes @jamescrosswell @vaind for thoughts

@kanadaj
Copy link
Contributor Author

kanadaj commented Dec 3, 2023

It still requires #ifdef on MAUI so long as the property is only available on Android. At the same time, of we don't do that, it will get confusing that the property is there on platforms without logcat

@bruno-garcia
Copy link
Member

It still requires #ifdef on MAUI so long as the property is only available on Android. At the same time, of we don't do that, it will get confusing that the property is there on platforms without logcat

right it makes sense the property is only available on Android. My learning from seeing this PR is that the usage of the Android-only option is a bit awkward if sitting directly on option when compared to having it under options.Android.Abc

It's easy to communicate and discover to:

#if ANDROID
options.Android... // Intellisense shows all options

vs somehow inside the #if inspecting the top level options properties and having to discover something "new" in the mix of a ton of other things that show regardless of platform

@bruno-garcia
Copy link
Member

bruno-garcia commented Dec 3, 2023

We are renaming Android to Native which frees up adding a new property Android (lol?) that would only only Android specific things.

As I mentioned before I don't know if this is the best way forward, but wanted to share these thoughts. @vaind wdyt?

@jamescrosswell
Copy link
Collaborator

My learning from seeing this PR is that the usage of the Android-only option is a bit awkward if sitting directly on option when compared to having it under options.Android.Abc

I agree. I wouldn't move it, tbh.

If we do want to make it always available (so the client code doesn't need #if directives) I think it's inconsistent with how we surface all the other platform specific options. So either we we do or we don't, but let's not do it for some options but not others.

@bruno-garcia
Copy link
Member

My learning from seeing this PR is that the usage of the Android-only option is a bit awkward if sitting directly on option when compared to having it under options.Android.Abc

I agree. I wouldn't move it, tbh.

If we do want to make it always available (so the client code doesn't need #if directives) I think it's inconsistent with how we surface all the other platform specific options. So either we we do or we don't, but let's not do it for some options but not others.

Now that the binding option became option.Native, we can create a SentryAndroidOptions class to add Android-only properties, and keep the original location.

Basically pulling main into this PR will force you into that set up since the Android to Native renaming was merged already

@bitsandfoxes
Copy link
Contributor

bitsandfoxes commented Dec 7, 2023

Making that PR now work with what got merged on main looks annoying. Sorry about that. I finished this over here #2959

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.

4 participants