-
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
Code cleanup #29012
Code cleanup #29012
Conversation
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 🐑 🇮🇹, though we should maybe discuss if we want target-typed everywhere systematically
new DatabaseModelFactoryOptions(tables, schemas), | ||
new ModelReverseEngineerOptions { UseDatabaseNames = useDatabaseNames, NoPluralize = noPluralize }, | ||
new ModelCodeGenerationOptions | ||
new(tables, schemas), |
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.
FWIW though I like target-typed construction, I'm not sure we should systematically always use it where possible - having the explicit type names makes it easier to understand what's going on in some contexts.
Maybe leave this on a case-by-case basis (i.e. no automated changing)?
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.
@@ -23,7 +23,9 @@ public class ModelCodeGeneratorSelector : LanguageBasedSelector<IModelCodeGenera | |||
/// </summary> | |||
public ModelCodeGeneratorSelector(IEnumerable<IModelCodeGenerator> services) | |||
: base(services.Except(services.OfType<TemplatedModelGenerator>()).ToList()) | |||
=> _templatedModelGenerators = services.OfType<TemplatedModelGenerator>().ToList(); | |||
{ |
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.
But why.... 😢
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.
Presumably because it is an assignment. There is probably a setting for this somewhere, if we care.
mappingStrategy, entityType.DisplayName())); | ||
} | ||
|
||
; |
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.
👀
8fdd61c
to
5508ac5
Compare
5508ac5
to
fbb4d9c
Compare
Fixes #28964