Code cleanup for src #36565
Code cleanup for src #36565
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.
}
| @@ -80,7 +80,7 @@ public static void SetContainer(this IMutableEntityType entityType, string? name | |||
| } | |||
|
|
|||
| 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
AndriySvyryd
force-pushed
the
SourceCleanup
branch
from
fac8485 to
af22225
Compare
| ? 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.
| @@ -80,7 +80,7 @@ public static void SetContainer(this IMutableEntityType entityType, string? name | |||
| } | |||
|
|
|||
| 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.
| @@ -57,7 +57,11 @@ protected override Expression VisitExtension(Expression expression) | |||
|
|
|||
| // Converts Offset and Limit parameters to constants when ORDER BY RANK is detected in the SelectExpression (i.e. we order by scoring function) | |||
| // 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.