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.

Sign up for GitHub

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

Jump to bottom

Try to embrace the future early #23296

Merged
merged 1 commit into from
Nov 18, 2020
Merged

Try to embrace the future early #23296

merged 1 commit into from
Nov 18, 2020

Conversation

bricelam
Copy link
Contributor

@smitpatel @roji feel free to start tackling the 775 new NRT warnings

bricelam
bricelam commented Nov 12, 2020
View reviewed changes
Directory.Build.props Outdated Show resolved Hide resolved
roji
roji reviewed Nov 12, 2020
View reviewed changes
src/EFCore.InMemory/EFCore.InMemory.csproj
@@ -2,7 +2,7 @@

<PropertyGroup>
<Description>In-memory database provider for Entity Framework Core (to be used for testing purposes).</Description>
<TargetFramework>netstandard2.1</TargetFramework>
<TargetFramework>net5.0</TargetFramework>
Copy link
Member

Choose a reason for hiding this comment

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

Should we extract this to the common Directory.Build.props or something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Personally I like to keep the csproj files relativly "vanilla" even if it's not as DRY. I hate hunting through msbuild files to find basic properties.

Copy link
Contributor

Choose a reason for hiding this comment

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

src/EFCore.Sqlite.Core/EFCore.Sqlite.Core.csproj Outdated Show resolved Hide resolved
test/EFCore.Cosmos.FunctionalTests/TestUtilities/CosmosDbConfiguredConditionAttribute.cs Outdated Show resolved Hide resolved
roji
roji reviewed Nov 17, 2020
View reviewed changes
Copy link
Member

@roji roji left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@smitpatel 👍

A few comments below

src/EFCore.InMemory/Query/Internal/InMemoryExpressionTranslatingExpressionVisitor.cs Show resolved Hide resolved
src/EFCore.InMemory/Query/Internal/InMemoryProjectionBindingExpressionVisitor.cs Show resolved Hide resolved
src/EFCore.InMemory/Query/Internal/InMemoryQueryableMethodTranslatingExpressionVisitor.cs Show resolved Hide resolved
src/EFCore/Query/EntityShaperExpression.cs Show resolved Hide resolved
src/EFCore/Query/EvaluatableExpressionFilter.cs Outdated Show resolved Hide resolved
src/EFCore/Query/Internal/NavigationExpandingExpressionVisitor.cs Show resolved Hide resolved
src/EFCore/Query/Internal/NavigationExpandingExpressionVisitor.cs Show resolved Hide resolved
src/EFCore/Query/Internal/QueryOptimizingExpressionVisitor.cs Outdated Show resolved Hide resolved
src/EFCore/Query/QueryCompilationContext.cs Show resolved Hide resolved
roji
roji reviewed Nov 17, 2020
View reviewed changes
src/EFCore/Metadata/Internal/PropertyBase.cs
[NotNull] string fieldName,
[NotNull] TypeBase type,
[CanBeNull] string propertyName,
[NotNull] string propertyName,
Copy link
Member

Choose a reason for hiding this comment

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

Not sure what the annotation for this method should actually be... Usage in our codebase doesn't seem to need nullable propertyName, and we do have checks on the return value being nullable.

src/EFCore/Metadata/Internal/SkipNavigationComparer.cs
return result != 0 ? result : EntityTypeFullNameComparer.Instance.Compare(x.DeclaringEntityType, y.DeclaringEntityType);
}
public int Compare(SkipNavigation? x, SkipNavigation? y)
=> (x, y) switch
Copy link
Member

Choose a reason for hiding this comment

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

For reference: https://twitter.com/smitpatel/status/1328545513549905920

@bricelam
Copy link
Contributor Author

LGTM

@bricelam bricelam marked this pull request as ready for review November 17, 2020 22:35
@smitpatel smitpatel self-assigned this Nov 17, 2020
@smitpatel
Copy link
Contributor

Failing for linux/mac 😨

@smitpatel
Copy link
Contributor

The error is coming from the file which is not in this PR (since AzDo will rebase on top of main to run PR). I am taking this PR over to make it review ready and merge. I will be rewriting history so please hold off pushing any changes for a day.

@smitpatel smitpatel force-pushed the feature/tfm branch 2 times, most recently from fc72110 to cd77d47 Compare November 17, 2020 23:59
@smitpatel
Copy link
Contributor

@AndriySvyryd - Ping!

AndriySvyryd
AndriySvyryd reviewed Nov 18, 2020
View reviewed changes
src/EFCore.SqlServer/Scaffolding/Internal/SqlServerDatabaseModelFactory.cs
@@ -188,7 +188,7 @@ static int GetEngineEdition(DbConnection connection)
using var command = connection.CreateCommand();
command.CommandText = @"
SELECT SERVERPROPERTY('EngineEdition');";
return (int)command.ExecuteScalar();
return (int)command.ExecuteScalar()!;
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 ! needed here? Isn't the cast enough?

Copy link
Contributor

Choose a reason for hiding this comment

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

Casting something which returns nullable to non-null with hard cast is still null error. 😆

Copy link
Member

Choose a reason for hiding this comment

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

@roji Is this by-design or a nullable bug?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, it's by design.

The language decision was indeed to require an explicit bang in any conversion from nullable to non-nullable - a cast is not enough. One good argument for that is that if casts were enough, then in legacy (unannotated) code they would automatically become nullable->non-nullable conversions, introducing many bugs.

Re ExecuteScalar, that is particularly painful. The problem is that ExecuteScalar does return null (as opposed to DBNull.Value) when there is no resultset, i.e. if you execute DELETE/UPDATE/INSERT. It's an awful API, and I hope to have enough time to make a better one in dotnet/runtime#26511, which would be both generic (non-boxing) and better around nullability.

Copy link
Member

@AndriySvyryd AndriySvyryd Nov 18, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I could argue that a cast to non-nullable value type would still retain its meaning, but I see why they wouldn't want complex rules like this

@dotnet dotnet deleted a comment from smitpatel Nov 18, 2020
AndriySvyryd
AndriySvyryd approved these changes Nov 18, 2020
View reviewed changes
Copy link
Member

@AndriySvyryd AndriySvyryd left a comment

Choose a reason for hiding this comment

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

I like it 👍

@smitpatel smitpatel merged commit 0b31650 into main Nov 18, 2020
@smitpatel smitpatel deleted the feature/tfm branch November 18, 2020 04:15
@mmitche mmitche mentioned this pull request Nov 18, 2020
Merged
@ajcvickers ajcvickers mentioned this pull request Nov 20, 2020
Closed
@bachratyg bachratyg mentioned this pull request Feb 16, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AndriySvyryd AndriySvyryd AndriySvyryd approved these changes

@roji roji roji left review comments

@smitpatel smitpatel smitpatel left review comments

Assignees

@smitpatel smitpatel

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@bricelam @smitpatel @roji @AndriySvyryd