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

fix: Read initWithDict for Hybrid SDKs #2579

Merged
merged 6 commits into from
Jan 9, 2023

Conversation

philipphofmann
Copy link
Member

📜 Description

Add SentryOptions:initWithDict only for Hybrid SDKs again.

#skip-changelog

💡 Motivation and Context

Fixes GH-2547

💚 How did you test it?

Unit tests.

📝 Checklist

  • I reviewed the submitted code
  • I added tests to verify the changes
  • I updated the docs if needed
  • Review from the native team if needed
  • No breaking changes

🔮 Next steps

Add SentryOptions:initWithDict only for Hybrid SDKs again.

Fixes GH-2547
@github-actions
Copy link

github-actions bot commented Jan 5, 2023

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1211.14 ms 1224.14 ms 13.00 ms
Size 20.76 KiB 409.70 KiB 388.94 KiB

Baseline results on branch: 8.0.0

Startup times

Revision Plain With Sentry Diff
58ec104 1244.29 ms 1269.67 ms 25.39 ms
766d549 1214.00 ms 1222.74 ms 8.74 ms
9f8d429 1239.57 ms 1255.12 ms 15.55 ms
6c7ab81 1231.66 ms 1251.58 ms 19.92 ms
53e149f 1213.62 ms 1253.40 ms 39.77 ms
c7d7a55 1198.46 ms 1227.64 ms 29.18 ms
10f5348 1257.04 ms 1266.52 ms 9.48 ms
1018de3 1251.20 ms 1254.06 ms 2.86 ms
88ac2c2 1223.04 ms 1243.12 ms 20.08 ms
b9c9598 1228.57 ms 1251.10 ms 22.53 ms

App size

Revision Plain With Sentry Diff
58ec104 20.75 KiB 379.11 KiB 358.36 KiB
766d549 20.76 KiB 406.02 KiB 385.26 KiB
9f8d429 20.75 KiB 383.40 KiB 362.65 KiB
6c7ab81 20.75 KiB 405.10 KiB 384.34 KiB
53e149f 20.75 KiB 402.12 KiB 381.36 KiB
c7d7a55 20.75 KiB 383.40 KiB 362.65 KiB
10f5348 20.75 KiB 402.11 KiB 381.36 KiB
1018de3 20.75 KiB 402.08 KiB 381.33 KiB
88ac2c2 20.75 KiB 373.61 KiB 352.86 KiB
b9c9598 20.75 KiB 383.77 KiB 363.01 KiB

Previous results on branch: feat/readd-init-with-dict

Startup times

Revision Plain With Sentry Diff
4f22767 1247.08 ms 1262.04 ms 14.96 ms
7fbf276 1220.20 ms 1238.34 ms 18.14 ms

App size

Revision Plain With Sentry Diff
4f22767 20.76 KiB 409.70 KiB 388.94 KiB
7fbf276 20.76 KiB 408.91 KiB 388.14 KiB

@codecov
Copy link

codecov bot commented Jan 5, 2023

Codecov Report

Merging #2579 (6470b59) into 8.0.0 (0188d70) will increase coverage by 0.29%.
The diff coverage is 97.57%.

❗ Current head 6470b59 differs from pull request most recent head 84b957f. Consider uploading reports for the commit 84b957f to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##            8.0.0    #2579      +/-   ##
==========================================
+ Coverage   79.09%   79.39%   +0.29%     
==========================================
  Files         243      243              
  Lines       22325    22446     +121     
  Branches     9871     9952      +81     
==========================================
+ Hits        17659    17821     +162     
+ Misses       4212     4172      -40     
+ Partials      454      453       -1     
Impacted Files Coverage Δ
Sources/Sentry/SentryOptions.m 96.78% <97.57%> (+9.57%) ⬆️
...es/Sentry/SentryWatchdogTerminationScopeObserver.m 70.51% <0.00%> (-2.43%) ⬇️
Sources/Sentry/SentryTracer.m 95.05% <0.00%> (-0.32%) ⬇️
Sources/Sentry/SentryHub.m 95.96% <0.00%> (-0.02%) ⬇️
Sources/Sentry/SentryPerformanceTracker.m 100.00% <0.00%> (ø)
Sources/Sentry/SentryNSTimerWrapper.m
Sources/Sentry/include/SentryHexAddressFormatter.h 100.00% <0.00%> (ø)
Sources/Sentry/SentryEvent.m 98.29% <0.00%> (+0.02%) ⬆️
Sources/Sentry/SentryBreadcrumbTracker.m 77.24% <0.00%> (+0.41%) ⬆️
Sources/Sentry/SentryUser.m 95.74% <0.00%> (+1.06%) ⬆️
... and 7 more

Continue to review full report at Codecov.

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

@brustolin brustolin changed the title fix: Readd initWithDict for Hybrid SDKs fix: Read initWithDict for Hybrid SDKs Jan 9, 2023
Copy link
Contributor

@brustolin brustolin left a comment

Choose a reason for hiding this comment

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

H: SentryOptions-HybridSDKs.h target membership needs to be private instead of project to expose it through Carthage for Hybrid SDKs that uses it.

image

L: I think we can #import "SentryOptions-HybridSDKs.h" in PrivateSentryOnlySDK.h to make things easier.

Sources/Sentry/Public/SentryOptions.h Show resolved Hide resolved
@philipphofmann
Copy link
Member Author

H: SentryOptions-HybridSDKs.h target membership needs to be private instead of project to expose it through Carthage for Hybrid SDKs that uses it.

Why doesn't CI complain about that? What can we do to avoid that mistake in the future by CI complaining?

@brustolin
Copy link
Contributor

Why doesn't CI complain about that? What can we do to avoid that mistake in the future by CI complaining?

Compiler is forgiven to headers not being used.
I guess to try this in CI we would need something importing this private headers. But then we need to remember to include the import for every new private header.

Copy link
Contributor

@brustolin brustolin left a comment

Choose a reason for hiding this comment

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

There is something wrong in the CHANGELOG.

Other than that, LGTM

@philipphofmann philipphofmann merged commit e6fd1fb into 8.0.0 Jan 9, 2023
@philipphofmann philipphofmann deleted the feat/readd-init-with-dict branch January 9, 2023 14:50
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.

Undo Options initWithDict removal
2 participants