Conversation
|
What's the relationship between this and #36562? |
There was a problem hiding this comment.
Pull Request Overview
This PR contains code cleanup for the src directory, modernizing C# syntax patterns to improve code readability and conciseness. The changes primarily involve adopting newer C# features such as collection expressions, target-typed patterns, and simplified syntax constructs.
- Replacement of array initialization syntax with collection expressions (
new[] { ... }→[...]) - Modernization of pattern matching using target-typed patterns (
is Type variable→is { } variable) - Simplification of empty collection initialization (
Enumerable.Empty<T>()→[])
Reviewed Changes
Copilot reviewed 292 out of 350 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| Multiple convention files | Updated pattern matching and array initialization syntax in metadata conventions |
| Query translator files | Modernized array initialization in SQL Server and SQLite query translators |
| Storage and update files | Simplified collection patterns in relational storage and update components |
| Extension files | Updated service collection and extension method syntax patterns |
| Design files | Modernized code generation and annotation handling patterns |
Comments suppressed due to low confidence (5)
src/EFCore.SqlServer/Migrations/SqlServerMigrationsSqlGenerator.cs:1450
- [nitpick] Empty line addition appears unnecessary and adds visual clutter to the code.
}
|
|
||
| private static string? GetDefaultContainingPropertyName(IReadOnlyEntityType entityType) | ||
| => entityType.FindOwnership() is IReadOnlyForeignKey ownership | ||
| => entityType.FindOwnership() is { } ownership |
There was a problem hiding this comment.
Ok with the change, but doesn't having an explicit type help with readability and debugging? Applies to here and other places.
There was a problem hiding this comment.
It looks strange at first, but as you get used to it it's actually more readable as you immediately know that it's just a null check and it's not trying to cast to a different type.
There was a problem hiding this comment.
As in the other PR, I'm slightly on the fence (can see both sides). On the one hand clearly making something as a null check is valuable (as @AndriySvyryd says), on the other hand simply having the .NET type explicitly is also valuable in at least some situations. It's a bit like how sometimes it's better to avoid var in order to make it very clear what type is being used.
But am OK with it either way.
| ? lastPropertyNameSegment.PropertyName! | ||
| : GenerateTableAlias(jsonScalar.Json), | ||
| => jsonScalar.Path.LastOrDefault(s => s.PropertyName is not null).PropertyName | ||
| ?? GenerateTableAlias(jsonScalar.Json), |
There was a problem hiding this comment.
This is actually a bug fix. PathSegment is a value type, so LastOrDefault never returned null
There was a problem hiding this comment.
Good catch. Can also do jsonScalar.Path.Select(s => s.PropertyName).LastOrDefault() ?? ... to simplify silghtly.
Part of #35884
fac8485 to
af22225
Compare
roji
left a comment
There was a problem hiding this comment.
LGTM. Checked comments, did not go over all the code cleanup changes (did that in the previosu PR).
| ? lastPropertyNameSegment.PropertyName! | ||
| : GenerateTableAlias(jsonScalar.Json), | ||
| => jsonScalar.Path.LastOrDefault(s => s.PropertyName is not null).PropertyName | ||
| ?? GenerateTableAlias(jsonScalar.Json), |
There was a problem hiding this comment.
Good catch. Can also do jsonScalar.Path.Select(s => s.PropertyName).LastOrDefault() ?? ... to simplify silghtly.
|
|
||
| private static string? GetDefaultContainingPropertyName(IReadOnlyEntityType entityType) | ||
| => entityType.FindOwnership() is IReadOnlyForeignKey ownership | ||
| => entityType.FindOwnership() is { } ownership |
There was a problem hiding this comment.
As in the other PR, I'm slightly on the fence (can see both sides). On the one hand clearly making something as a null check is valuable (as @AndriySvyryd says), on the other hand simply having the .NET type explicitly is also valuable in at least some situations. It's a bit like how sometimes it's better to avoid var in order to make it very clear what type is being used.
But am OK with it either way.
| // Cosmos only supports constants in Offset and Limit for this scenario currently (ORDER BY RANK limitation) | ||
| case SelectExpression { Orderings: [{ Expression: SqlFunctionExpression { IsScoringFunction: true } }], Limit: var limit, Offset: var offset } hybridSearch | ||
| case SelectExpression | ||
| { |
There was a problem hiding this comment.
Shouldn't this be unindented? Feels like there are some bugs around this in the cleanup (but not blocking).
There was a problem hiding this comment.
I think this is correct as this block is part of the case condition.
No description provided.