Enable nullable reference type warnings#207
Conversation
|
Hi @FantasyTeddy ... thanks for your PR. I have not run a build so far. Are these all the nullable errors? Can we quickly outline what your plan is ;) |
| //we only want to discover Lego-devices | ||
| //filter for correct service-GUID for LEGO-Hub, containing manufacturerData and the name | ||
| var addressNumber = BlueGigaBLEHelper.ByteArrayToUlong(e.Address); | ||
| var bleAdvertisingListFound = bleParsedData.TryGetValue(addressNumber, out var actualListAdvertisingData); |
There was a problem hiding this comment.
With the additional variable, the compiler can no longer correctly infer that actualListAdvertisingData cannot be null if TryGetValue returns true.
| var dumpStaticPortInfoCommand = scopedServiceProvider.GetRequiredService<DumpStaticPortInfo>(); // ServiceLocator ok: transient factory | ||
|
|
||
| var port = byte.Parse(portOption.Value()); | ||
| var port = byte.TryParse(portOption.Value(), out var parsedPort) ? parsedPort : (byte)0; |
There was a problem hiding this comment.
Is not that a behavior change to the CLI?
There was a problem hiding this comment.
I guess technically, yes. But wasn't the behavior before that omitting the -p port option would lead to an exception here?
| public Hub CreateByBluetoothManufacturerData(byte[] manufacturerData) | ||
| { | ||
| var hub = (manufacturerData is null || manufacturerData.Length < 3) ? null : Create(GetTypeFromSystemType(GetSystemTypeFromManufacturerData((PoweredUpHubManufacturerData)manufacturerData[1]))); | ||
| if (manufacturerData is null) |
There was a problem hiding this comment.
I am not sure if this is right change. Maybe the function needs a Hub? as a result. Obviously the init line needs to change.
There was a problem hiding this comment.
I just tried to keep the changes minimal.
Before this change, the call to Configure later would throw a NullReferenceException anyway, and I figured that throwing a more meaningful exception would be better.
Sure, sorry for dumping a bunch of PRs without any real explanation. I would love to enable nullable reference types in this project to catch some potential bugs and also help users to better understand the contract behind some of the code. |
|
Nullable is a good path. Let us do this. But we should finish it before the 6.0 release then. I do not want hundreds of warnings in the build. |
FantasyTeddy
force-pushed
the
nullable-warnings
branch
from
8079888 to
9fa9367
Compare
|
I would love that, but there are currently no warnings from that first step, because I tried to fix them. |
|
How bad is it? Or do you need me to take a shot at it? |
|
There are quite a lot if we enable it fully. I had to give up, due to a lack of understanding of the functionality. |
2 participants
This could be a first step towards enabling nullable reference types.
#205 non-breaking