-
-
Notifications
You must be signed in to change notification settings - Fork 205
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
refactor: rename native options #2940
Conversation
|
||
#### Changed APIs | ||
|
||
- Rename iOS and MacCatalyst platform specific options from `Cocoa` to `Native` ([#2940](https://github.com/getsentry/sentry-dotnet/pull/2940)) |
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.
Note to final 4.0.0 changelog rewording: outside the beta there was no Cocoa
, it was iOS
to Native
(right?)
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.
Yes and that is also true for many other parts of the 4.0 changelog. we need to update it and review thoroughly before the final release.
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.
We could keep track of these things in a ticket (we already have #2673)?
@@ -9,17 +9,17 @@ internal class AndroidEventProcessor : ISentryEventProcessor, IDisposable | |||
private readonly JavaSdk.IEventProcessor? _androidProcessor; | |||
private readonly JavaSdk.Hint _hint = new(); | |||
|
|||
public AndroidEventProcessor(SentryAndroidOptions androidOptions) | |||
public AndroidEventProcessor(SentryAndroidOptions nativeOptions) |
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.
Should class SentryAndroidOptions
also become SentryNativeOptions
or that's rather confusing because sentry-native
?
Although I see it being renamed below, did we have 2 SentryAndroidOptions
s?
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.
This refers to Java class name
@@ -6,7 +6,7 @@ | |||
|
|||
<ItemGroup> | |||
<Using Include="Foundation" /> | |||
<Using Include="Sentry.CocoaSdk.SentryOptions" Alias="SentryCocoaOptions" /> | |||
<Using Include="Sentry.CocoaSdk.SentryOptions" Alias="SentryCocoaSdkOptions" /> |
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.
How about?
<Using Include="Sentry.CocoaSdk.SentryOptions" Alias="SentryCocoaSdkOptions" /> | |
<Using Include="Sentry.CocoaSdk.SentryOptions" Alias="SentryCocoaOptions" /> |
Not sure the namespace is CocoaSdk
, could it be Cocoa
? Do we call JavaSdk
or AndroidSdk
on the other binding?
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.
This is related to the native cocoa SDK generated binding code. I didn't want to touch that internal bit
closes #2934