ref: Make description in SpanProtocol nonnull

[philipphofmann]

📜 Description

SentrySpanProtocol.startChildWithOperation has an overload without a
description if you don't want to specify a description. We don't need
the description to be nullable.

💡 Motivation and Context

Improves the API.

💚 How did you test it?

CI.

📝 Checklist

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

🔮 Next steps

[github-actions]

	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 under the following heading:

To the changelog entry, please add a link to this PR (consider a more descriptive message):`

- Make description in SpanProtocol nonnull(#1059)

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

Generated by 🚫 dangerJS against b370a21

[codecov-commenter]

Codecov Report

Merging #1059 (b370a21) into master (64cd981) will increase coverage by 0.06%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1059      +/-   ##
==========================================
+ Coverage   94.14%   94.20%   +0.06%     
==========================================
  Files          93       93              
  Lines        4115     4127      +12     
==========================================
+ Hits         3874     3888      +14     
+ Misses        241      239       -2     

Impacted Files	Coverage Δ
Sources/Sentry/SentrySpan.m	100.00% <100.00%> (ø)
Sources/Sentry/SentryCrashIntegration.m	92.48% <0.00%> (+0.67%)	⬆️
Sources/Sentry/SentryUser.m	98.30% <0.00%> (+1.69%)	⬆️
Sources/Sentry/SentryThreadInspector.m	100.00% <0.00%> (+3.70%)	⬆️

---

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 64cd981...b370a21. Read the comment docs.

[bruno-garcia]

This was a conscious decision and we discussed it on the PR

The problem with this (and I hit this problem using the API in .NET) is that if you are calling this with a "string variable" for description which you don't know if it's null or not, you need to do a If check to call one overload or the other.

This code was done in .NET (the way you're proposing here) and it's just not a good API.

description is nullable, so makes sense and it's a better usability to have the overload be nullable

[philipphofmann]

Wouldn't you agree that you don't see nullable in method overloads rarely? We usually don't do this in the SDK

SentrySDK.capture(event: , scope: )
SentrySDK.capture(event: , block: )
SentrySDK.capture(message: , scope: )
SentrySDK.capture(message: , block: )
// ...

Attachment(data: , filename: , contentType: )

I also don't see this often in standard libraries, so for me, it looks weird. I'm a huge fan of the null safety for example in Kotlin and for me, it makes sense to use it rarely. In Swift per default, every value is nonnull as well and you have to declare it as optional if you want it to be null. Again it makes sense to me to use nullable judiciously.

The problem with this (and I hit this problem using the API in .NET) is that if you are calling this with a "string variable" for description which you don't know if it's null or not, you need to do a If check to call one overload or the other.

You would have this problem with every method overload where the parameters are nonnull. I think in this case you should make sure the description can't be null. It forces you to give it a value when you use the description. Therefore for me, it is actually a good API, because it avoids nullable values.

[marandaneto]

Wouldn't you agree that you don't see nullable in method overloads rarely? We usually don't do this in the SDK

SentrySDK.capture(event: , scope: )
SentrySDK.capture(event: , block: )
SentrySDK.capture(message: , scope: )
SentrySDK.capture(message: , block: )
// ...

Attachment(data: , filename: , contentType: )

I also don't see this often in standard libraries, so for me, it looks weird. I'm a huge fan of the null safety for example in Kotlin and for me, it makes sense to use it rarely. In Swift per default, every value is nonnull as well and you have to declare it as optional if you want it to be null. Again it makes sense to me to use nullable judiciously.

The problem with this (and I hit this problem using the API in .NET) is that if you are calling this with a "string variable" for description which you don't know if it's null or not, you need to do a If check to call one overload or the other.

You would have this problem with every method overload where the parameters are nonnull. I think in this case you should make sure the description can't be null. It forces you to give it a value when you use the description. Therefore for me, it is actually a good API, because it avoids nullable values.

obviously, this is a trade-off, both use cases here have pros/cons, what makes me decide if a nullable field should have a non-nullable field in the method param is if this could be used at runtime dynamically, eg description is likely being used with a SQL query or literally anything else, including a request or response value, which could be null/empty or not, so it's just easier to allow them doing do(x, y) instead of if (x & y) call do(x, y) instead do(x).

[brustolin]

I'm late to the discussion but I agree that if the property can be null, its value in any method should allow null.