Skip to content

enable nullable for new projects #32756

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

Closed
wants to merge 5 commits into from
Closed

Conversation

jmarolf
Copy link
Contributor

@jmarolf jmarolf commented May 16, 2021

@jmarolf jmarolf requested a review from a team as a code owner May 16, 2021 23:03
@ghost ghost added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label May 16, 2021
@jmarolf jmarolf force-pushed the enable-nullable branch from d8a9ac7 to 19776ac Compare May 17, 2021 02:24
@mkArtakMSFT mkArtakMSFT requested a review from pranavkm May 24, 2021 21:36
@mkArtakMSFT
Copy link
Contributor

@danroth27 can you please confirm that we want to take this change in for our templates? Thanks!

@pranavkm
Copy link
Contributor

I would love to enable nullability, but I think we would enable it by default rather than make it a switch that a user has to choose. That said, I think there are some important pieces we would need to address before we did that:

  • Nullability for identity UI so that it scaffolds well
  • Update scaffolding for controllers / ef /razor
  • Fix some Razor code generation bugs where it inadvertently suppresses nullability in user code.

The remainder of the framework is annotated enough (and we will cover more by 6.0) that users should find the experience productive.

@jmarolf
Copy link
Contributor Author

jmarolf commented Jun 1, 2021

@pranavkm should this be closed or is there some set of projects we think its appropriate for?

@danroth27
Copy link
Member

I would love to enable nullability, but I think we would enable it by default rather than make it a switch that a user has to choose.

👍

I think there are some important pieces we would need to address before we did that:

Do we have tracking issues for these remaining work items?

@jmarolf
Copy link
Contributor Author

jmarolf commented Jun 1, 2021

I would love to enable nullability, but I think we would enable it by default rather than make it a switch that a user has to choose.

it is enabled by default. The defaultValue is true. The user needs to explicitly pass false

@captainsafia
Copy link
Member

@danroth27 There's #32783

@danroth27
Copy link
Member

@jmarolf I think the question is do we need a switch at all. Once we are at a place with ASP.NET Core that we can turn on nullability checks by default, is there really a need for users to have a switch to turn it off?

@jmarolf
Copy link
Contributor Author

jmarolf commented Jun 1, 2021

I think the question is do we need a switch at all

I have no objections. I added the switch as a conservative measure. If we are comfortable just using this as the default that makes things simpler.

@@ -7,7 +7,7 @@
<h1 class="text-danger">Error.</h1>
<h2 class="text-danger">An error occurred while processing your request.</h2>

@if (Model.ShowRequestId)
@if (Model?.ShowRequestId ?? false)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks kinda terrible.

@DamianEdwards
Copy link
Member

Nullable was enabled by the other changes that went in today.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable nullable by default for new projects
8 participants