-
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
Fixes around namespaces and Fluent API calls #25435
Conversation
src/EFCore.Design/Scaffolding/Internal/CSharpDbContextGenerator.cs
Outdated
Show resolved
Hide resolved
src/EFCore.Design/Scaffolding/Internal/CSharpDbContextGenerator.cs
Outdated
Show resolved
Hide resolved
d8ce5d5
to
ad51db0
Compare
Co-authored-by: Andriy Svyryd <AndriySvyryd@users.noreply.github.com>
@@ -1421,7 +1315,7 @@ protected virtual void GenerateKeyAnnotations(IKey key, IndentedStringBuilder st | |||
Check.NotNull(annotations, nameof(annotations)); | |||
Check.NotNull(stringBuilder, nameof(stringBuilder)); | |||
|
|||
foreach (var annotation in annotations) | |||
foreach (var annotation in annotations.OrderBy(a => a.Name)) |
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.
Why do you need to reorder?
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 removed this method which was dead code (#25435 (comment)), but the same ordering is applied to raw annotations in the new GenerateAnnotations. It's for alphabetical determinism etc., nothing too important.
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 wonder whether using SortedDictionary
would be faster since they were already ordered initially.
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.
Interesting idea... Hard to know - we'd likely pay more in other accesses/lookups (e.g. when generating the fluent APIs, when removing ignorable/by-convention annotations...), who knows which way it goes.
I've seen lots of things around code generation here which aren't very optimized - that's probably OK. If we have indication that things are slow, at some point we can profile a bit and get rid of the worst offenders etc...
(BTW perf is also why I decided to discovery namespaces in scaffolding via a single pass along with generation, rather than the approach used in MigrationsCodeGenerator of a 2nd pass only for namespace discovery. Not sure how much all this is important...)
src/EFCore.Design/Scaffolding/Internal/CSharpDbContextGenerator.cs
Outdated
Show resolved
Hide resolved
src/EFCore.Design/Scaffolding/Internal/CSharpDbContextGenerator.cs
Outdated
Show resolved
Hide resolved
src/EFCore.Design/Scaffolding/Internal/CSharpDbContextGenerator.cs
Outdated
Show resolved
Hide resolved
src/EFCore.Design/Scaffolding/Internal/CSharpDbContextGenerator.cs
Outdated
Show resolved
Hide resolved
a46bc8e
to
b91f109
Compare
This reverts commit c4c2822.
Thanks @AndriySvyryd! |
@bricelam this is a draft for getting your general OK before going further... #25046 (custom namespaces in scaffolding) is fully implemented, while #23848 (type-qualified Fluent APIs in the model snapshot) is a bit more tricky/ugly.
Closes #25046
Closes #23848
Closes #25434