0️⃣ Start enabling nullable #195
Conversation
| @@ -2,9 +2,11 @@ | |||
| <PropertyGroup> | |||
| <TargetFrameworks>net472;netcoreapp2.2</TargetFrameworks> | |||
| <LangVersion>8</LangVersion> | |||
| <Nullable>warnings</Nullable> | |||
| <Nullable>enable</Nullable> | |||
| <WarningsAsErrors>CS8600;CS8602;CS8603</WarningsAsErrors> | |||
There was a problem hiding this comment.
From what I could find, errors need to be enabled manually. Would like to know if there's a better way.
There was a problem hiding this comment.
A quick search pointed me to the fact you may be able to replace these 3 codes with the word "nullable"
There was a problem hiding this comment.
This is great, thanks! Turns out this causes a ton of other errors, so I will leave it at the above warnings for now and submit further PRs to achieve full nullability for easier review.
| @@ -234,7 +234,7 @@ public static T RandomElement<T>(this IEnumerable<T> source, Random random) | |||
| { | |||
| throw new InvalidOperationException("Sequence was empty."); | |||
| } | |||
| return current; | |||
| return current!; | |||
There was a problem hiding this comment.
The above check on count ensures that this will never be null (unless in fact the sequence contained a null value, in which case that is the intended behaviour).
There was a problem hiding this comment.
Optional:
Same comment as in the other PR, but it may not be a bad practice to always add a comment documenting cases where we use ! in a non-obvious way in code to avoid future confusion.
| @@ -129,7 +129,7 @@ public T RunAndReturn<T>(Func<T> action) | |||
| { | |||
| var ret = default(T); | |||
| RunAndAwait(() => ret = action()); | |||
| return ret; | |||
| return ret!; | |||
There was a problem hiding this comment.
The implementation of RunAndAwait ensures that the return value will be set. It may of course be set to null, if that is what the action returns, but that is entirely up to the caller.
| @@ -2,9 +2,11 @@ | |||
| <PropertyGroup> | |||
| <TargetFrameworks>net472;netcoreapp2.2</TargetFrameworks> | |||
| <LangVersion>8</LangVersion> | |||
| <Nullable>warnings</Nullable> | |||
| <Nullable>enable</Nullable> | |||
| <WarningsAsErrors>CS8600;CS8602;CS8603</WarningsAsErrors> | |||
There was a problem hiding this comment.
A quick search pointed me to the fact you may be able to replace these 3 codes with the word "nullable"
| @@ -234,7 +234,7 @@ public static T RandomElement<T>(this IEnumerable<T> source, Random random) | |||
| { | |||
| throw new InvalidOperationException("Sequence was empty."); | |||
| } | |||
| return current; | |||
| return current!; | |||
There was a problem hiding this comment.
Optional:
Same comment as in the other PR, but it may not be a bad practice to always add a comment documenting cases where we use ! in a non-obvious way in code to avoid future confusion.
| @@ -129,7 +129,7 @@ public T RunAndReturn<T>(Func<T> action) | |||
| { | |||
| var ret = default(T); | |||
| RunAndAwait(() => ret = action()); | |||
| return ret; | |||
| return ret!; | |||
|
Explaining the null forgiving operators made me find another issue: #197 so that's great. I'd like to merge it like it is now (after the bin packing PR), and then submit another PR with all nullable errors fixed (or perhaps multiple if there are other things that require big changes). |
✨ What's this?
This enables nullable as errors in all projects and makes the necessary changes.
🔗 Relationships
Ref #184
Ref #194 (a pre-requisite for this PR)
🔍 Why do we want this?
Nullability is hip.
🏗 How is it done?
I enabled nullable and made sure they end up as errors in all project files. Then it was as simple as hunting errors and fixing case by case until everything compiled again.
💥 Breaking changes
This is a breaking change for anyone who has nullable enabled themselves, as some of our public members are now explicitly nullable.
🔬 Why not another way?
This is the most straight forward. The changes are pretty minor and splitting it into multiple PRs seemed silly, with the exception of #194, which includes several major changes.
💡 Review hints
I'll leave comments on all changes to explain why they are correct. Feel free to review based on that or however else you like.
Note that until #194 is merged into master and this PR brought up to date, the main project will not build. There is no conflict between the changes however.