-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Generate change-tracking delegates in the compiled model. #31443
Conversation
f1f199e
to
1f1e4b3
Compare
78d8e80
to
1a531e7
Compare
src/EFCore.Design/Query/Internal/LinqToCSharpSyntaxTranslator.cs
Outdated
Show resolved
Hide resolved
/// 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 static readonly MethodInfo PopulateListMethod |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this move to a common, public, location?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It points to a method on a pubternal class. To me it makes sense for it to be pubternal as well.
FYI starting to review (especially the LinqToCSharp changes). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed the LinqToCSharpSyntaxTranslator changes; overall looks good but there are several things we should discuss (we can jump into a call tomorrow if you want).
src/EFCore.Design/Query/Internal/ILinqToCSharpSyntaxTranslator.cs
Outdated
Show resolved
Hide resolved
src/EFCore.Design/Query/Internal/LinqToCSharpSyntaxTranslator.cs
Outdated
Show resolved
Hide resolved
src/EFCore.Design/Query/Internal/LinqToCSharpSyntaxTranslator.cs
Outdated
Show resolved
Hide resolved
src/EFCore.Design/Query/Internal/LinqToCSharpSyntaxTranslator.cs
Outdated
Show resolved
Hide resolved
src/EFCore.Design/Query/Internal/LinqToCSharpSyntaxTranslator.cs
Outdated
Show resolved
Hide resolved
src/EFCore.Design/Query/Internal/LinqToCSharpSyntaxTranslator.cs
Outdated
Show resolved
Hide resolved
src/EFCore.Design/Query/Internal/LinqToCSharpSyntaxTranslator.cs
Outdated
Show resolved
Hide resolved
src/EFCore.Design/Query/Internal/LinqToCSharpSyntaxTranslator.cs
Outdated
Show resolved
Hide resolved
src/EFCore.Design/Query/Internal/LinqToCSharpSyntaxTranslator.cs
Outdated
Show resolved
Hide resolved
1a531e7
to
a71232e
Compare
src/EFCore.Design/Query/Internal/LinqToCSharpSyntaxTranslator.cs
Outdated
Show resolved
Hide resolved
src/EFCore.Design/Query/Internal/LinqToCSharpSyntaxTranslator.cs
Outdated
Show resolved
Hide resolved
/// </summary> | ||
protected virtual ExpressionSyntax GenerateValue(object? value) | ||
{ | ||
return value switch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: expression-bodied
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't, contains local functions that you like so much
src/EFCore.Design/Query/Internal/LinqToCSharpSyntaxTranslator.cs
Outdated
Show resolved
Hide resolved
// return NullableType(Translate(type.GenericTypeArguments[0])); | ||
//} | ||
|
||
var generic = GenericName( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need this logic to produce fully-qualified names, to avoid the usual ambiguities etc. (unrelated to this PR).
src/EFCore.Design/Query/Internal/RuntimeModelLinqToCSharpSyntaxTranslator.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aside from the compound assignment issue for private fields, LGTM.
Part of #29761
Still missing private constructors and service property bindings