-
-
Notifications
You must be signed in to change notification settings - Fork 329
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
fix: Double-quoted include, expected angle-bracketed instead. #4298
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4298 +/- ##
=============================================
+ Coverage 91.659% 91.669% +0.010%
=============================================
Files 617 617
Lines 50236 50238 +2
Branches 18150 18148 -2
=============================================
+ Hits 46046 46053 +7
+ Misses 4098 4093 -5
Partials 92 92 see 4 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
1bf8571 | 1250.96 ms | 1255.36 ms | 4.40 ms |
236d8a8 | 1232.33 ms | 1255.55 ms | 23.22 ms |
b9b0f0a | 1235.61 ms | 1237.40 ms | 1.79 ms |
94e1968 | 1230.22 ms | 1253.33 ms | 23.11 ms |
0341c52 | 1235.76 ms | 1246.67 ms | 10.91 ms |
fc163f5 | 1198.05 ms | 1227.76 ms | 29.71 ms |
dbc67d2 | 1239.49 ms | 1248.88 ms | 9.39 ms |
dc0fe58 | 1198.56 ms | 1220.98 ms | 22.42 ms |
f5623cd | 1255.78 ms | 1262.30 ms | 6.52 ms |
9dbf743 | 1252.10 ms | 1262.10 ms | 10.00 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
1bf8571 | 20.76 KiB | 437.12 KiB | 416.36 KiB |
236d8a8 | 21.58 KiB | 418.70 KiB | 397.12 KiB |
b9b0f0a | 20.76 KiB | 434.93 KiB | 414.17 KiB |
94e1968 | 21.58 KiB | 614.74 KiB | 593.15 KiB |
0341c52 | 21.58 KiB | 544.86 KiB | 523.27 KiB |
fc163f5 | 20.76 KiB | 436.30 KiB | 415.54 KiB |
dbc67d2 | 20.76 KiB | 427.74 KiB | 406.98 KiB |
dc0fe58 | 20.76 KiB | 436.50 KiB | 415.74 KiB |
f5623cd | 22.85 KiB | 412.98 KiB | 390.13 KiB |
9dbf743 | 20.76 KiB | 434.94 KiB | 414.18 KiB |
Before merging this, I would like to discuss whether we want to maintain the need to have the following code snippet in most public headers: if __has_include(<Sentry/Sentry.h>)
# import <Sentry/SentryDefines.h>
#else
# import <SentryWithoutUIKit/SentryDefines.h>
#endif Having virtually two frameworks in the same codebase causes this type of abnormality. |
Yeah, that's for sure annoying. The only alternative I can imagine is to split the framework so that we have a core module and a UIKit module that can be optionally linked and would be done so by default. Per my previous recommendations, we would keep our existing package manager spec name for sentry cocoa which would pull in both sentry-cocoa-core and sentry-cocoa-uikit, and then people that do not want the uikit module would directly target sentry-cocoa-core instead of sentry-cocoa. In the future, sentry-cocoa would also be the umbrella target for any other modules we decide to split out, like sentry-cocoa-crash, sentry-cocoa-performance etc. |
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.
Agreed this approach doesn't scale, but if it unsticks us now let's do it and try to prioritize splitting out a separate UIKIt module.
💡 Motivation and Context
closes #3972
💚 How did you test it?
CI
📝 Checklist
You have to check all boxes before merging:
sendDefaultPII
is enabled.🔮 Next steps