Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Nullable to Automapping #678

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

bethmaloney
Copy link
Contributor

@bethmaloney bethmaloney commented Apr 13, 2024

This PR begins the process of adding nullability support to fluent-nhibernate.

Nullable Reference is a compiler feature that provides warning of potential null reference errors. This feature has been enabled by default since .NET Core 6 and many 3rd party libraries have since added support for this feature.

I've began by enabling Nullable in the Automapping folder and resolving any errors. Most of the work was simply adding annotations but I did occasionally have to add a null check or refactor the code to allow the static analyser to determine that something was not nullable.

You'll note that there is a few Activator.CreateInstance(type)! with the null forgiving operator. Activator.CreateInstance will only return a null for nullable value types which we never activate (they're all classes) so it's safe to use the null forgiving operator.

This PR requires #668 to be merged in as .Net framework/standard does not include the nullability attributes in the standard library so we can't add support without adding a third party NuGet library that adds these attributes to the lower versions.

I'm using the pre-processor directive

#if USE_NULLABLE
#nullable enable
#endif

This allows us to disable nullability for release builds until the migration is complete. Once the migration is complete we can remove the directives.

Let me know if there's any issues with this approach. If there's not I'm happy to migrate the rest of fluent-nhibernate over.

@bethmaloney
Copy link
Contributor Author

DeepSource is failing due to a code smell in an area of code I haven't modified. Not sure if I should be fixing the issue or if we can ignore it?

@hazzik
Copy link
Member

hazzik commented Apr 13, 2024

Thanks for the contribution.

This PR requires #668 to be merged

No, it is not needed. The project uses Polyfill that provides all required attributes.

@bethmaloney
Copy link
Contributor Author

My understanding is that polyfill will make the nullable attributes available but don't add them to the base library. Eg in .net 8 string.IsNullOrEmpty will mark a string as being non null. However in .net 4.6 this won't happen as the function is missing the required attributes.

We'd need some sort of package that could go and rewrite/add the attributes to the base library. Otherwise we'll get nullability errors in earlier versions of .net.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants