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

Enable nullable reference types in the solution and evaluate code (OSOE-754) #227

Open
sarahelsaig opened this issue Dec 9, 2023 · 5 comments

Comments

@sarahelsaig
Copy link
Member

sarahelsaig commented Dec 9, 2023

Enabling nullable mode for the HL would make the code much safer and give us more confidence about where additional null checks are needed or not. It's also painful to implement abstract methods from HL that should be able to return null in a nullable enabled project because the IDE gets confused.

Also the codebase should be reviewed to guarantee that the methods only return null when it has a special meaning (e.g. in drivers) and to consider making the inputs as resilient as possible.

Jira issue

@github-actions github-actions bot changed the title Enable nullable reference types in the solution and evaluate code Enable nullable reference types in the solution and evaluate code (OSOE-754) Dec 9, 2023
@Piedone
Copy link
Member

Piedone commented Dec 11, 2023

Duplicate of Lombiq/.NET-Analyzers#48, right?

@sarahelsaig
Copy link
Member Author

No. Some time ago, we discussed that it's okay to enable nullable in new projects even before it's globally enabled via Lombiq.Analyzers. I think it's even more applicable to HL for the reasons I mentioned above and because that reduces the intimidation factor of Lombiq/.NET-Analyzers#48, that will necessarily involve making all of our projects use nullable references first. So this issue could be considered a blocker for it.

@Piedone
Copy link
Member

Piedone commented Dec 11, 2023

Yeah, good point.

@Piedone
Copy link
Member

Piedone commented Sep 4, 2024

For this, Lombiq.HelpfulLibraries.Refit needs to be moved to .NET 8 from .NET Standard. This is not an issue, since it was required only for Lombiq.OrchardCoreApiClient, which is now .NET 8 too.

A Directory.Build.props file like this could be placed into the repo roo to enable for all projects:

<Project>
  <PropertyGroup>
    <Nullable>enable</Nullable>
  </PropertyGroup>
</Project>

@Piedone
Copy link
Member

Piedone commented Sep 5, 2024

This should rather be done after we upgraded to Orchard Core 2.0, to prevent an insance merge conflict.

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

No branches or pull requests

2 participants