Skip to content

Add nullable to DataProtection #22591

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

Merged
merged 2 commits into from
Jun 14, 2020
Merged

Conversation

pranavkm
Copy link
Contributor

@pranavkm pranavkm commented Jun 5, 2020

No description provided.

@Pilchie Pilchie added the area-dataprotection Includes: DataProtection label Jun 8, 2020
@pranavkm pranavkm force-pushed the prkrishn/nullable-dataprotection branch from 48f17fc to ba16200 Compare June 10, 2020 02:37
@pranavkm pranavkm marked this pull request as ready for review June 10, 2020 16:00
@pranavkm pranavkm added the api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews label Jun 10, 2020
@ghost
Copy link

ghost commented Jun 10, 2020

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

  • The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
  • The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
  • Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

@@ -7,6 +7,8 @@
<AllowUnsafeBlocks>true</AllowUnsafeBlocks>
<GenerateDocumentationFile>true</GenerateDocumentationFile>
<PackageTags>aspnetcore;dataprotection</PackageTags>
<Nullable>enable</Nullable>
<Nullable Condition="'$(TargetFramework)' == 'netstandard2.0'">annotations</Nullable>
Copy link
Member

Choose a reason for hiding this comment

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

Why is there a seemingly random choice of enable or annotations for each csproj, and why is this the only multi-targeted one that does this logic?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In general, we can only turn on nullable = annotations for ns2.0 projects. All ns2.0 APIs are oblivious, so if you rely on BCL's api for nullability, it doesn't work. In this case, we rely on annotations in Debug.Assert. Most of the other projects here are implementation heavy, changing it is going to take a lot of work, which is they are annotation only.

@pranavkm pranavkm force-pushed the prkrishn/nullable-dataprotection branch from ba16200 to e1418ec Compare June 14, 2020 00:04
@pranavkm pranavkm linked an issue Jun 14, 2020 that may be closed by this pull request
@pranavkm pranavkm merged commit f04992f into master Jun 14, 2020
@pranavkm pranavkm deleted the prkrishn/nullable-dataprotection branch June 14, 2020 03:39
@pranavkm pranavkm removed the api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews label Mar 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-dataprotection Includes: DataProtection
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update ASP.NET Core to use C# 8's nullable reference types
5 participants