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

feat: allow partial sdk info override #1816

Merged
merged 4 commits into from
May 10, 2022
Merged

feat: allow partial sdk info override #1816

merged 4 commits into from
May 10, 2022

Conversation

vaind
Copy link
Collaborator

@vaind vaind commented May 10, 2022

📜 Description

💡 Motivation and Context

Turns out we want to keep the sentry-cocoa SDK version when overriding the name while integrated in the sentry-unity SDK. This PR enables doing that.

💚 How did you test it?

new 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

@github-actions
Copy link

github-actions bot commented May 10, 2022

Messages
📖 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against 42bec3c

Sources/Sentry/Public/SentrySdkInfo.h Outdated Show resolved Hide resolved
@@ -44,6 +44,25 @@ - (instancetype)initWithDict:(NSDictionary *)dict
return [self initWithName:name andVersion:version];
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I wanted to reuse the new method with sth like:

return [self initWithDict:dict orDefaults:nil];

but that would require the arg to be nullable - do you want me to make the change?

Copy link
Member

Choose a reason for hiding this comment

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

I would prefer if we could reuse the new method. We could also define an internal constructor that both initWithDict: and initWithDict:orDefaults use. Then we don't need to make the orDefaults nullable.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

tried here, not sure that's the right way in objective-c though: 80edcb8

@codecov-commenter
Copy link

codecov-commenter commented May 10, 2022

Codecov Report

Merging #1816 (42bec3c) into master (e5a906b) will decrease coverage by 0.05%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1816      +/-   ##
==========================================
- Coverage   92.16%   92.10%   -0.06%     
==========================================
  Files         198      198              
  Lines        9062     9070       +8     
==========================================
+ Hits         8352     8354       +2     
- Misses        710      716       +6     
Impacted Files Coverage Δ
Sources/Sentry/Public/SentrySdkInfo.h 100.00% <ø> (ø)
Sources/Sentry/SentryOptions.m 99.57% <100.00%> (ø)
Sources/Sentry/SentrySdkInfo.m 100.00% <100.00%> (ø)
Sources/Sentry/SentryProfilingLogging.mm 71.42% <0.00%> (-14.29%) ⬇️
Sources/Sentry/SentryThreadHandle.cpp 71.94% <0.00%> (-0.72%) ⬇️
Sources/Sentry/SentryMachLogging.cpp 2.56% <0.00%> (-0.52%) ⬇️
Sources/Sentry/SentryBacktrace.cpp 92.63% <0.00%> (ø)

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 e5a906b...42bec3c. Read the comment docs.

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.

LGTM

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. The test that is failing is flaky. We are working on fixing it #1756

@vaind vaind merged commit e7957bf into master May 10, 2022
@vaind vaind deleted the feat/sdk-info-partial branch May 10, 2022 16:35
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.

5 participants