Refactoring: Dart Nullsafety #298
Conversation
Codecov Report
@@ Coverage Diff @@
## main #298 +/- ##
==========================================
- Coverage 86.52% 80.97% -5.55%
==========================================
Files 52 5 -47
Lines 1751 226 -1525
==========================================
- Hits 1515 183 -1332
+ Misses 236 43 -193 Continue to review full report at Codecov.
|
dart/lib/src/protocol/message.dart
Outdated
| class Message { | ||
| /// The fully formatted message. If missing, Sentry will try to interpolate the message. | ||
| final String formatted; | ||
| final String? formatted; |
There was a problem hiding this comment.
this is required https://develop.sentry.dev/sdk/event-payloads/message/
There was a problem hiding this comment.
I would leave it as optional because it gets filled from nullable fields, such as SentryClient.captureMessage(...) and the message could be null.
There was a problem hiding this comment.
SentryClient.captureMessage(null) should not raise an event at all, it's not going to be useful.
if we create a Message without a formatted field, it'll cause a backend error during ingestion.
There was a problem hiding this comment.
just reforcing this, captureMessage(String?) is fine, but we'll bail out before creating a Message, so formatted should be non nulable to not generate a backend error
There was a problem hiding this comment.
I think captureMessage shouldn't take null
The code can gracefully handle a null (programming error) but the annotation should be that a message is required.
For all the public API when we need to value to do something meaningful
There was a problem hiding this comment.
this would give a lot of boilerplate or force an error on the users code, think about?
final str = getthisfromApi();
// str is nullable
Sentry.captureMessage(str!); // could cause an error on app's code.
// doing this would be annoying
if (str != null) {
Sentry.captureMessage(str!);
}
There was a problem hiding this comment.
as a Message is created only after capturing, hence the field formatted should not be nullable
Yep, I've added the explanation to your review. But for all other which don't want to search: |
got it, thanks, hard to get these things during code review if not trying out :) |
…/nullsafety # Conflicts: # dart/lib/src/scope.dart
|
@ueman I've created the |
…/nullsafety # Conflicts: # dart/lib/src/hub.dart # dart/lib/src/hub_adapter.dart # dart/lib/src/noop_hub.dart # dart/lib/src/noop_sentry_client.dart # dart/lib/src/sentry_client.dart # dart/test/hub_test.dart # dart/test/sentry_client_test.dart
| String formatted, | ||
| String template, | ||
| List<dynamic> params, | ||
| String? formatted, |
There was a problem hiding this comment.
| sentry: ">=4.0.0 <5.0.0" | ||
| package_info: ^0.4.0 | ||
| # this should point to pub | ||
| sentry: ^4.0.5 |
There was a problem hiding this comment.
would not be better to keep between compatible versions?
There was a problem hiding this comment.
It has to be changed anyway before you release this even if I change it to ">=4.0.0 <5.0.0". As Sentry should probably be 5.0.0-prerelease.0 and at that time there is no sentry with a 5.0.0 release. There's also no reason to release a sentry_flutter because it's not null safe yet.
There was a problem hiding this comment.
Or am I overlooking something?
There was a problem hiding this comment.
we can discuss versioning in another PR, lets merge it, my question was more about, why changing from >= ... < to ^...
|
We're now at a point where the discussion is more about the API and if it should take nulls or not than the underlying null safety changes. Does it make sense to merge this and do these changes yourself? I'm not quite sure I am the right person to decide on it 😅 |
|
Sounds good to me but it's up to @marandaneto. |
|
there are only 3 comments open 2 of them are the same, about Message.formatted that should not be nullable. |
marandaneto
left a comment
marandaneto
left a comment
There was a problem hiding this comment.
@ueman thanks a lot for this, a lot of back and forth but its also a change in the whole code base so yeah.
happy to merge :)
|
Also thanks alot for the review. |
4 participants
📜 Description
This PR migrates the Dart part of this Sentry SDK to null safety.
Some notes:
dynamiccan hold null values by default, even without a?A lot of test were touched so they should be reviewed carefully.
Most of the user facing APIs accept null values. However events, messages or exceptions are getting dropped if they are null.
So the question is should the API allow null values? I would guess so because it makes the API a little bit more user friendly but I'm kinda torn on that. For now null values are allowed.
💡 Motivation and Context
#247
💚 How did you test it?
📝 Checklist
🔮 Next steps
5.0.0-nullsafety.0according to https://dart.dev/null-safety/migration-guide