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

ref: Make public APIs Swift friendly #2416

Merged
merged 10 commits into from
Nov 23, 2022

Conversation

brustolin
Copy link
Contributor

📜 Description

  • Renamed SentrySDK.addBreadcrumb(crumb:) to SentrySDK.addBreadcrumb(_ crumb:)
  • Renamed SentryClient.add(_ crumb:) to SentryClient.addBreadcrumb(_ crumb:)
  • Renamed SentryClient.add(_ attachment:) to SentryClient.addAttachment(_ attachment:)
  • Renamed SentryClient.apply(to:) to SentryClient.applyTo(session:)
  • Renamed SentryClient.apply(to:maxBreadcrumb:) to SentryClient.applyTo(event:maxBreadcrumbs:)

💡 Motivation and Context

close #1844

💚 How did you test it?

📝 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

@brustolin brustolin changed the base branch from master to 8.0.0 November 21, 2022 11:00
@github-actions
Copy link

github-actions bot commented Nov 21, 2022

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1242.35 ms 1267.04 ms 24.69 ms
Size 20.75 KiB 381.87 KiB 361.12 KiB

Baseline results on branch: 8.0.0

Startup times

Revision Plain With Sentry Diff
88ac2c2 1223.04 ms 1243.12 ms 20.08 ms
9be1db2 1219.42 ms 1245.66 ms 26.24 ms
4dc66f6 1202.59 ms 1228.34 ms 25.75 ms
7eee302 1228.73 ms 1241.94 ms 13.21 ms
d10145a 1232.65 ms 1257.55 ms 24.90 ms
bbe81cb 1257.25 ms 1272.24 ms 14.99 ms
e2cec76 1189.48 ms 1229.84 ms 40.36 ms
1ce879f 1258.12 ms 1260.90 ms 2.78 ms
68094b3 1214.14 ms 1255.09 ms 40.95 ms
58ec104 1244.29 ms 1269.67 ms 25.39 ms

App size

Revision Plain With Sentry Diff
88ac2c2 20.75 KiB 373.61 KiB 352.86 KiB
9be1db2 20.75 KiB 373.94 KiB 353.19 KiB
4dc66f6 20.75 KiB 381.81 KiB 361.06 KiB
7eee302 20.75 KiB 374.73 KiB 353.97 KiB
d10145a 20.75 KiB 379.12 KiB 358.36 KiB
bbe81cb 20.75 KiB 381.81 KiB 361.06 KiB
e2cec76 20.75 KiB 381.81 KiB 361.06 KiB
1ce879f 20.75 KiB 381.81 KiB 361.06 KiB
68094b3 20.75 KiB 373.94 KiB 353.19 KiB
58ec104 20.75 KiB 379.11 KiB 358.36 KiB

Previous results on branch: ref/public-APIs-Swift-friendly

Startup times

Revision Plain With Sentry Diff
6d6483d 1214.14 ms 1241.30 ms 27.16 ms
fcc055a 1230.80 ms 1251.82 ms 21.02 ms
45b6ce4 1206.27 ms 1232.74 ms 26.47 ms
4b855dc 1245.16 ms 1271.83 ms 26.67 ms
5156bfc 1217.41 ms 1243.94 ms 26.53 ms
b1ab6c7 1231.92 ms 1242.94 ms 11.02 ms
26a56f3 1236.59 ms 1260.72 ms 24.13 ms
8ed4514 1266.18 ms 1271.78 ms 5.60 ms

App size

Revision Plain With Sentry Diff
6d6483d 20.75 KiB 379.12 KiB 358.36 KiB
fcc055a 20.75 KiB 379.11 KiB 358.36 KiB
45b6ce4 20.75 KiB 379.11 KiB 358.36 KiB
4b855dc 20.75 KiB 379.12 KiB 358.36 KiB
5156bfc 20.75 KiB 379.12 KiB 358.36 KiB
b1ab6c7 20.75 KiB 381.87 KiB 361.12 KiB
26a56f3 20.75 KiB 379.12 KiB 358.36 KiB
8ed4514 20.75 KiB 379.11 KiB 358.36 KiB

@kevinrenskers
Copy link
Contributor

  • Renamed SentryClient.apply(to:) to SentryClient.applyTo(session:)
  • Renamed SentryClient.apply(to:maxBreadcrumb:) to SentryClient.applyTo(event:maxBreadcrumbs:)

Big fan of most changes, but these don't feel like good improvements to me. SentryClient.apply(to:) feels more "Swifty".

@brustolin
Copy link
Contributor Author

Big fan of most changes, but these don't feel like good improvements to me. SentryClient.apply(to:) feels more "Swifty".

I find it odd the two apply functions have a total different kind of target.
In this case in particular they also have different signatures, so, someone familiar with the SDK may know the target just by looking.
But I think we should keep "clarity". Maybe apply(toSession:) apply(toEvent:).

@kevinrenskers
Copy link
Contributor

Ah sorry I missed the part where one applies to a session and the other to an event. I think your change makes sense, but I also like your latest suggestion 👍

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.

Thanks for doing this, @brustolin.

@kevinrenskers kevinrenskers merged commit 2ae7db9 into 8.0.0 Nov 23, 2022
@kevinrenskers kevinrenskers deleted the ref/public-APIs-Swift-friendly branch November 23, 2022 09:28
@kevinrenskers
Copy link
Contributor

I've merged this since it was constantly getting merge conflicts. More upcoming changes can be in a separate PR.

@github-actions
Copy link

Fails
🚫 Please consider adding a changelog entry for the next release.

Instructions and example for changelog

Please add an entry to CHANGELOG.md to the "Unreleased" section. Make sure the entry includes this PR's number.

Example:

## Unreleased

- Make public APIs Swift friendly ([#2416](https://github.com/getsentry/sentry-cocoa/pull/2416))

If none of the above apply, you can opt out of this check by adding #skip-changelog to the PR description.

Generated by 🚫 dangerJS against 6ab21e8

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.

Check public APIs for Swift friendliness
3 participants