AdamsLair / duality

a 2D Game Development Framework
https://adamslair.github.io/duality
MIT License
1.41k stars 287 forks source link

Consider enabling nullable references #724

Open Barsonax opened 5 years ago

Barsonax commented 5 years ago

Summary

Enabling nullable references will help spot null reference bugs while also providing better self documenting code to endusers.

Analysis

Barsonax commented 5 years ago

698 is a prerequisite of this

ilexp commented 5 years ago

I'm all for nullable reference tagging 👍 There will be some work to do regarding the C# update, as I'm not a fan of introducing all the modern C# features into the codebase from a style perspective, but as soon as that's out of the way, we can start annotating.

ilexp commented 5 years ago

Since this requires C# 8.0, which requires .NET Core 3.0, I'm moving this to the future upgrade milestone.

Barsonax commented 5 years ago

C#8.0 does not require .NET Core 3.0. Msbuild 16.x is enough.

ilexp commented 5 years ago

I've so far only found sources that state it does, at least regarding official support, see here for example:

Because C# 8.0 has been built for .NET Core 3.0 and .NET Standard 2.1, it will not be supported outside of .NET Core 3.0 and any platform implementing .NET Standard 2.1.

Do you have an official docs / blog statement that clears things up, or updates this with newer info?

Barsonax commented 5 years ago

I believe thats because some features rely on some framework types and runtime enhancements. However a feature like nullable references does not (though its still in preview and subject to change). Iam using C# 8.0 in Singularity https://github.com/Barsonax/Singularity/blob/master/src/Singularity/Singularity.csproj

I think the best course of action is to wait a bit more for now since we have more work to do and nullable references are still in a bit of a preview state (even though previously I stated it was not) but my guess is that you can use C# 8.0 without problems except that not all language features are available but I believe that nullable references alone are enough reason to eventually adopt C# 8.0.

ilexp commented 5 years ago

I think the best course of action is to wait a bit more for now since we have more work to do and nullable references are still in a bit of a preview state [..]

Yeah, I'm thinking something similar. I've extracted all C# 8.0 items into a separate milestone that we can tackle in the future. The C# 7.3 / .NET Standard one can be continued and done immediately and is a prerequisite for all the rest anyway.

[..] but my guess is that you can use C# 8.0 without problems except that not all language features are available [..]

I'm worried about maintenance implications though, since we'd depart from what's officially supported and can't easily take it back once we're depending on C# 8.0 features. An alternative that might be preferable would be to actually switch to .NET Core 3.0 for launcher and editor alongside the C# 8.0 upgrade once they're all released and stable - needs to be done "at some point" anyway with .NET Framework being superseded by .NET 5. Either way, I think we can just see about that when we get to it.

Barsonax commented 5 years ago

I'm worried about maintenance implications though, since we'd depart from what's officially supported and can't easily take it back once we're depending on C# 8.0 features.

Agreed thats why I think its better to wait a bit for now. Lots of other stuff to fix/improve anyway and maybe by that time its more clear if this is a safe route to take or not or else we just wait for .NET Core 3.0.