Feat/enable nullable reference types core #52
Conversation
Dovchik
changed the title
Dovchik
requested review from
650elx,
krogers0607,
asein-sinch and
JPPortier
as code owners
|
|
||
| ISinchAuth auth = | ||
| new OAuth(_keyId, _keySecret, _httpClient, _loggerFactory?.Create<OAuth>(), | ||
| new OAuth(_keyId!, _keySecret!, _httpClient, _loggerFactory?.Create<OAuth>(), |
There was a problem hiding this comment.
I'm not sure about using the null-forgiving operator here as apart from logging a warning, there is no guarantee to have a non-null value at this point
There was a problem hiding this comment.
See reply to another comment: https://github.com/sinch/sinch-sdk-dotnet/pull/52/files#r1568851411
|
|
||
| return new SmsClient( | ||
| new ProjectId( | ||
| _projectId!), // exception is throw when trying to get SMS client property if _projectId is null |
There was a problem hiding this comment.
It's strange to say that you are sure the _projectId is not null here but comment that is may still be null and it would be handled later on in the code
There was a problem hiding this comment.
Null forgiving operator (!) is not about to be sure something is not null, but to suppress warnings which is necessary for the case - same applies to the OAuth. Null refs is just another tools analysis and not a panacea to fix null ref exceptions, unfortunately
There was a problem hiding this comment.
The goal of the ! is to override the compiler analysis so that it would not issue a warning.
If I look at the SmsClient constructor, I see that ProjectId is a record which expects a non-null value. As there is no check here that prevents the _projectId variable to be null, maybe it would be clearer to set the record parameter as nullable such as string? Value and handle the null-value case in the SmsClient constructor before initializing the API classes (Batches, Inbounds...)
There was a problem hiding this comment.
This is by design, specific clients needs no nulls to function properly, but the caller is responsible for that and manages delayed/null initialization
Added test for |
First part of enabling null ref types. I will split the PR into different part for each service.