-
-
Notifications
You must be signed in to change notification settings - Fork 229
Update bindings for Cocoa SDK v9 #4830
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
base: deps/modules/sentry-cocoa.properties
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## deps/modules/sentry-cocoa.properties #4830 +/- ##
========================================================================
+ Coverage 73.76% 73.80% +0.03%
========================================================================
Files 483 483
Lines 17551 17551
Branches 3461 3461
========================================================================
+ Hits 12947 12954 +7
+ Misses 3748 3745 -3
+ Partials 856 852 -4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
509f0c9 to
fe091a7
Compare
76b917d to
1c85ec4
Compare
1c85ec4 to
dc6c357
Compare
c56e53c to
dd04bd0
Compare
dd04bd0 to
cf215b1
Compare
It has multiple conflicting NSNumber[] constructors for boolean, integer, and float arrays: error CS0111: Type 'SentryAttribute' already defines a member called 'Constructor' with the same parameter types
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.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
jamescrosswell
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.
Nice work - thanks @jpnurmi !
dc6c357 to
f4ed890
Compare
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.
question:
This replaces the above auto-generated PR, right?
| .WithAttribute("SentryBeforeSendEventCallback", "return: NullAllowed") | ||
| .WithAttribute("SentryTracesSamplerCallback", "return: NullAllowed") | ||
| // Fix nullable return attributes | ||
| .RemoveAttribute("PrivateSentrySDKOnly", "Capture*", "NullAllowed") |
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.
question:
Should we be explicitly removing the NullAllowed attribute for CaptureScreenshots and CaptureViewHierarchy, since we are adding [return: NullAllowed] on both of these methods below? Or are there more?
I am wondering if we could accidentally miss some new Capture* methods in the future.
| public bool EnableAppHangTracking { get; set; } = false; | ||
|
|
||
| /// <summary> | ||
| /// IMPORTANT: This feature is experimental and may have bugs. |
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.
suggestion: change phrasing
-/// IMPORTANT: This feature is experimental and may have bugs.
+/// IMPORTANT: App Hangs V2 is now the default.(didn't do a GitHub suggestion, as the result looked incorrect)
| /// As of version 8.39.0-beta.1 of the sentry-cocoa SDK, you can enable AppHangsV2, which is available on iOS and tvOS. | ||
| /// The main difference is that AppHangsV2 differentiates between fully-blocking and non-fully-blocking |
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.
suggestion: add clarification
Add the updated text from: https://docs.sentry.io/platforms/apple/configuration/app-hangs/#app-hangs-v2
/// As of version 8.39.0-beta.1 of the sentry-cocoa SDK, you can enable AppHangsV2, which is available on iOS and tvOS.
+/// Starting with version 9.0.0, App Hangs V2 is enabled by default.
/// The main difference is that AppHangsV2 differentiates between fully-blocking and non-fully-blocking(I didn't do a GitHub suggestion, as the result looked a bit weird in formatting)
| /// See https://docs.sentry.io/platforms/apple/configuration/app-hangs/#app-hangs-v2 | ||
| /// </remarks> | ||
| [Obsolete("Use EnableAppHangTracking")] | ||
| public bool EnableAppHangTrackingV2 { get; set; } = false; |
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.
question: forward to EnableAppHangTracking
Since has no effect anymore,
shall we "forward" it to EnableAppHangTracking?
Like
public bool EnableAppHangTrackingV2 { get => EnableAppHangTracking; set => EnableAppHangTracking = value; }Or do we indeed want to make this option essentially no-op, since this option is now Obsolete.
In this case, I'd make the message more clear:
[Obsolete("This option has no effect. Use EnableAppHangTracking instead.")]
//or maybe
[Obsolete("App Hangs V2 is now the default. Use EnableAppHangTracking instead.")]or something similar.
I'm a bit uncertain whether we should
- make
EnableAppHangTrackingV2no-op - make
EnableAppHangTrackingV2an alias toEnableAppHangTracking
Considering that we are releasing the Cocoa SDK major version in a minor .NET SDK version,
what is the "least breaking" option?
Also considering all combinations/permutations, that the user might have:
options.EnableAppHangTracking = true|false:
options.EnableAppHangTrackingV2 = true|false;I'm quite uncertain, I'm afraid.
| /// <remarks> | ||
| /// See https://docs.sentry.io/platforms/apple/configuration/app-hangs/ | ||
| /// </remarks> | ||
| public bool EnableAppHangTracking { get; set; } = false; |
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.
question: default to true?
See https://docs.sentry.io/platforms/apple/configuration/app-hangs/:
Starting with version 8.0.0, this feature has been enabled by default.
But also considering, that we are releasing this changeset as a minor version,
perhaps we do not want to change the default yet,
but instead open an issue to align the default with our v7 end of the year.
Or ... to we want to keep this default to false in general?
| options.IdleTimeout = IdleTimeout ?? options.IdleTimeout; | ||
| options.EnableAppHangTracking = EnableAppHangTracking ?? options.EnableAppHangTracking; | ||
| #pragma warning disable CS0618 // Type or member is obsolete | ||
| options.EnableAppHangTrackingV2 = EnableAppHangTrackingV2 ?? options.EnableAppHangTrackingV2; |
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.
question: obsolete
Should we mark this option (BindableSentryOptions.EnableAppHangTrackingV2) also as obsolete?
Or is that not necessary as this type is internal, and we won't "forget" to remove it for our next major.
| /// <remarks> | ||
| /// See https://docs.sentry.io/platforms/apple/configuration/app-hangs/#app-hangs-v2 | ||
| /// </remarks> | ||
| [Obsolete("Use EnableAppHangTracking")] |
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.
question: v7?
Do we plan to remove this option for our next major - v7 - in November 2026?
Shall we create an issue already, and mark it for "Major v7" in Linear?
f4ed890 to
117130f
Compare
Note
PrivatesHeader.handPrivateSentrySDKOnly.hwere moved fromPrivateHeaders/toHeaders/SentryOptions.enableAppHangTrackingV2was removed: chore: Remove hang tracker sdk v9 checks sentry-cocoa#6334SentryOptions.inAppExcludeswas removed: fix: Remove property that had no effect sentry-cocoa#6646PrivateSentrySDKOnly.getDebugImageswas removed: ref: Remove v9 checks for debug image provider sentry-cocoa#6454#skip-changelog (merges to #4781)