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

EFCore: Enable nullable on the project #24265

Merged
1 commit merged into from
Feb 26, 2021
Merged

EFCore: Enable nullable on the project #24265

1 commit merged into from
Feb 26, 2021

Conversation

smitpatel
Copy link
Member

Part of #19007
Left nullable enable to make review easier. Can replace all those with regex once we are done with all projects

@smitpatel smitpatel requested a review from a team February 25, 2021 00:57
src/EFCore.Relational/Metadata/ITable.cs Show resolved Hide resolved
src/EFCore/DbSet.cs Show resolved Hide resolved
src/EFCore/EFCore.csproj Show resolved Hide resolved
src/EFCore/Internal/DbContextPool.cs Outdated Show resolved Hide resolved
src/EFCore/Internal/DbContextServices.cs Outdated Show resolved Hide resolved
src/EFCore/Internal/EntityFinder.cs Outdated Show resolved Hide resolved
src/EFCore/Internal/EntityFinder.cs Show resolved Hide resolved
@@ -173,7 +173,7 @@ public override LocalView<TEntity> Local
/// any release. You should only use it directly in your code with extreme caution and knowing that
/// doing so can result in application failures when updating to a new Entity Framework Core release.
/// </summary>
public override TEntity Find(params object[] keyValues)
public override TEntity? Find(params object[]? keyValues)
Copy link
Member

Choose a reason for hiding this comment

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

Is it useful to allow the user to pass null here when we'll always just return null? Is there some reason to keep this (apart from the brekaing change)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Converted to object?[]?

Copy link
Member Author

Choose a reason for hiding this comment

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

Leaving it as unresolved if someone else know reason.

Copy link
Member

Choose a reason for hiding this comment

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

We should throw in the real DbSet if the array is null. (Or, indeed, if any of its values are null.)

Copy link
Member Author

Choose a reason for hiding this comment

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

We return null right now. A real DbSet passes off to EntityFinder and it returns null currently rather than throw.

src/EFCore/ValueGeneration/HiLoValueGenerator.cs Outdated Show resolved Hide resolved
src/EFCore/ValueGeneration/ValueGenerator`.cs Outdated Show resolved Hide resolved
@smitpatel smitpatel force-pushed the smit/MakeShayHappy branch 2 times, most recently from dba8ce0 to 3cb3ea7 Compare February 25, 2021 23:44
Base automatically changed from smit/MakeShayHappy to main February 26, 2021 09:28
Part of #19007

Left nullable enable to make review easier. Can replace all those with regex once we are done with all projects
@smitpatel
Copy link
Member Author

Also incorporated feedback from previous PR.

@ghost
Copy link

ghost commented Feb 26, 2021

Hello @smitpatel!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@ghost ghost merged commit dea57ad into main Feb 26, 2021
@ghost ghost deleted the smit/randomness branch February 26, 2021 20:59
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants