-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
3.1.11: Disable NRTs in scaffolding #23060
Conversation
@roji Approved for 3.1.11 by Tactics. Please wait until the branch is open for 3.1.11 before merging. |
Will this affect usage of the scaffolded code with .NET Standard 2.0 projects in a bad way? |
Good point @ErikEJ. 5.0 targets netstandard2.1 so this makes sense there, but depending on the of netstandard2.0 users this could be disruptive. @ajcvickers let's cancel backporting this? |
As I understand, .NET Standard 2.0 does not support C# 8 and 7.3 does not understand this (see linked comment) |
Yeah, it's possible (but unsupported) to use C# 8 with netstandard2.0, but for many people out there the language version is 7.3 (the default), and if #nullable disable starts to appear that won't compile. |
@roji Let's discuss this in triage tomorrow. |
@dotnet/efteam Should we still do this on 3.1, but change it to a comment? |
a57206d
to
f57176c
Compare
Approved by Tactics for 3.1 with the |
Original PR merged for 5.0: #21190
Note for Tactics
This issue was previously approved for 3.1.11. However, our ever astute MVP @ErikEJ pointed out that since 3.1 targets .NET Standard 2.0, we will potentially be generating
in projects where that won't compile.
Therefore, we could either not do this at all, or instead generate something like:
This backports the fix for #23016 to 3.1.
Description
This adds
#nullable disable
at the top of all C# files generated by EF Core's scaffolding.Customer Impact
EF Core scaffolding is currently unaware of C# 8.0 Nullable Reference Types (NRT), and generates a code model without annotations. If the project has NRTs turned on, the model get incorrectly interpreted, i.e. nullable database text columns are represented by
string SomeProperty
, which with NRTs is interpreted as non-nullable.How found
Reported by users (e.g. #23016). The issue for adding fully supporting NRTs, i.e. correctly scaffolding code with annotations (#15520) also has 28 votes (making it the 29th most requested feature), giving reason to believe many users are running into this.
Test coverage
This PR includes testing that the
#nullable disable
actually gets scaffolded.Regression?
No.
Risk
Low. This touches scaffolding code only - which does not affect EF Core runtime behavior - and in a minor way.
(replaces #23053 which had a persistent CI issue)