-
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
Add Comment to DatabaseColumn and DatabaseTable #16379
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.
This looks good to me. @bricelam Do you want to review?
src/EFCore.SqlServer/Scaffolding/Internal/SqlServerDatabaseModelFactory.cs
Outdated
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.
I'd add the corresponding code in RelationalScaffoldingModelFactory.VisitColumn
to the same PR. Seems incomplete to query for it and not use it.
@bricelam Question: For scaffolding with DataAnnotations, is the plan to use [Description] attribute? |
I don't think we have a corresponding annotation yet; we should file an issue. So the plan for now would be to scaffold |
@bricelam I have updated RelationalScaffoldingModelFactory - but unsure if those changes are enough - and fail to find a relevant test. |
Thanks! I'll look into testing |
b62aae0
to
1dbdc87
Compare
(Added some tests, made C# generation better, and rebased) |
Ah, just realized this is for entity type too 😄 Want to add tests for those and fix that C#? |
@bricelam I have tried 🤡 |
But tests are failing.. |
@@ -159,13 +159,32 @@ public void Comments_use_fluent_api() | |||
}), | |||
new ModelCodeGenerationOptions(), | |||
code => Assert.Contains( | |||
".HasColumn(\"An int property\")", |
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.
🤦♂ My bad.
93d6298
to
cda0092
Compare
Updated. We needed to pass the fully-qualified name to |
Great!!! |
src/EFCore.Design/Scaffolding/Internal/CSharpDbContextGenerator.cs
Outdated
Show resolved
Hide resolved
Updated - test failures seem unrelated |
Scaffold comment from SQL Server MS_Description
Add test and fix test Fix wrong class name Add missing test
e039e15
to
1c675ca
Compare
and Scaffold comment from SQL Server MS_Description
Part of #16305