-
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
Attribute to add comments on the table and columns #20676
Attribute to add comments on the table and columns #20676
Conversation
ralmsdeveloper
commented
Apr 18, 2020
- Resolve: Data annotation for configuring comments #20569
Could anyone on the team tell me if this would be a good way to go? /cc: @ajcvickers |
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.
Generally this is the right approach. But we don't think you should use the existing DataAnnotations DescriptionAttribute
: that would cause anyone who is currently using that to have these table / column comments show up - which is probably not what they intended. Instead could you create a new EntityFrameworkCore attribute similar to KeylessAttribute please?
You'd probably want this attribute to apply to Class, Property and Field (and could you add some tests for this latter as well please?).
But generally looking good. Thanks!
src/EFCore.Relational/Metadata/Conventions/RelationalTableDescriptionAttributeConvention.cs
Outdated
Show resolved
Hide resolved
test/EFCore.Relational.Tests/Metadata/RelationalEntityTypeAttributeConventionTest.cs
Outdated
Show resolved
Hide resolved
src/EFCore.Relational/Metadata/Conventions/RelationalColumnDescriptionAttributeConvention.cs
Outdated
Show resolved
Hide resolved
Thanks @lajones for your comments, but we have a small detail, the idea of following the KeylessAttribute, I feel comfortable, is that maybe when designing an architecture people may not want to dependence on EFCore in their domain. But I will continue with your suggestion. |
We should use: What are your thoughts on that? |
@ralmsdeveloper Personally I prefer |
src/EFCore.Relational/Metadata/Conventions/RelationalColumnCommentAttributeConvention.cs
Outdated
Show resolved
Hide resolved
test/EFCore.Relational.Tests/Metadata/RelationalEntityTypeAttributeConventionTest.cs
Outdated
Show resolved
Hide resolved
test/EFCore.Relational.Tests/Metadata/RelationalPropertyAttributeConventionTest.cs
Outdated
Show resolved
Hide resolved
@lajones I hope this time I got it right :) |
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.
Two minor changes and it'll all be good.
test/EFCore.Relational.Tests/Metadata/RelationalPropertyAttributeConventionTest.cs
Outdated
Show resolved
Hide resolved
cfebe4b
to
1ee8f69
Compare
Merged. Thanks for your contribution, @ralmsdeveloper |
Is it ready to use? |
@yuanxiongwei Yes. It's available in the latest preview, 5. |
@ajcvickers all right? |