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

chore: Move SentryDefine enums to swift #3963

Merged
merged 5 commits into from
May 17, 2024
Merged

Conversation

brustolin
Copy link
Contributor

We can't use Objective-C enums from Swift as a function argument without breaking TestUtil compilation, but we can use Swift int enums from Objective-C.

Session Replay will need SentryLevel, so a move it to swift, alongside with it I also moved SentryTransactionNameSource, both from SentryDefines.h

#skip-changelog

Copy link

codecov bot commented May 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.865%. Comparing base (0af45a9) to head (84e78a0).

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #3963       +/-   ##
=============================================
+ Coverage   90.848%   90.865%   +0.017%     
=============================================
  Files          594       594               
  Lines        46494     46494               
  Branches     16637     16659       +22     
=============================================
+ Hits         42239     42247        +8     
- Misses        4075      4177      +102     
+ Partials       180        70      -110     
Files Coverage Δ
Sources/Sentry/SentryBreadcrumb.m 97.530% <ø> (ø)
Sources/Sentry/SentryLevelMapper.m 100.000% <ø> (ø)
Sources/Sentry/SentryTransaction.m 89.230% <ø> (ø)
Sources/Sentry/include/SentryLog.h 89.473% <ø> (ø)
...ts/SentryTests/SentryKSCrashReportConverterTests.m 97.233% <ø> (ø)
Tests/SentryTests/SentryScopeTests.m 100.000% <ø> (ø)
Tests/SentryTests/SentryTests.m 100.000% <ø> (ø)

... and 31 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0af45a9...84e78a0. Read the comment docs.

@brustolin brustolin marked this pull request as draft May 9, 2024 15:05
@brustolin
Copy link
Contributor Author

So, this causes another problem.
Swift enums used in objc class properties or function parameters cannot be accessed from Swift.
This does not look good but I believe this is a step into more Swift code, until almost everything is swift.

Copy link
Member

@armcknight armcknight left a comment

Choose a reason for hiding this comment

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

What's keeping this in draft vs ready for review? You want to move more things?

Sources/Sentry/Public/SentryDefines.h Show resolved Hide resolved
@brustolin
Copy link
Contributor Author

What's keeping this in draft vs ready for review? You want to move more things?

After discovering the following problem

Swift enums used in objc class properties or function parameters cannot be accessed from Swift.

Im trying to figure it out if this is ok or not.

@brustolin
Copy link
Contributor Author

Im trying to figure it out if this is ok or not.

So, we still need to do this. Using objc enums from Swift cause cocoapod static linking to fail.
IMO we will need to move more classes to Swift faster.

@brustolin brustolin self-assigned this May 10, 2024
@brustolin brustolin marked this pull request as ready for review May 10, 2024 18:42
@philipphofmann
Copy link
Member

IMO we will need to move more classes to Swift faster.

Yes, indeed 💯

Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1235.82 ms 1252.86 ms 17.04 ms
Size 21.58 KiB 631.82 KiB 610.24 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
6bc5ae5 1230.90 ms 1246.10 ms 15.20 ms
a9103fe 1265.47 ms 1267.37 ms 1.90 ms
eef4553 1248.12 ms 1252.37 ms 4.24 ms
d0deebd 1235.17 ms 1255.54 ms 20.37 ms
6bc7c9c 1225.88 ms 1244.76 ms 18.88 ms
9fa5d27 1230.65 ms 1255.08 ms 24.43 ms
282cc99 1232.59 ms 1245.88 ms 13.29 ms
9f0d9e0 1212.49 ms 1220.27 ms 7.78 ms
ce4cfaf 1203.61 ms 1218.86 ms 15.25 ms
3a01b17 1212.12 ms 1221.80 ms 9.68 ms

App size

Revision Plain With Sentry Diff
6bc5ae5 20.76 KiB 401.40 KiB 380.64 KiB
a9103fe 20.76 KiB 426.95 KiB 406.19 KiB
eef4553 21.58 KiB 418.00 KiB 396.42 KiB
d0deebd 21.58 KiB 542.28 KiB 520.69 KiB
6bc7c9c 21.58 KiB 573.72 KiB 552.14 KiB
9fa5d27 20.76 KiB 393.37 KiB 372.61 KiB
282cc99 22.85 KiB 414.09 KiB 391.24 KiB
9f0d9e0 21.58 KiB 424.28 KiB 402.70 KiB
ce4cfaf 20.76 KiB 423.19 KiB 402.43 KiB
3a01b17 20.76 KiB 436.33 KiB 415.57 KiB

@brustolin brustolin merged commit 699d76f into main May 17, 2024
70 checks passed
@brustolin brustolin deleted the chore/move-enum-to-swift branch May 17, 2024 12:48
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.

None yet

3 participants