-
Notifications
You must be signed in to change notification settings - Fork 227
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
New EF Core comment support #950
Conversation
Note: I may hold off merging this until dotnet/efcore#16798 is fixed, to cover all cases. |
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.
Looks good, but could be improved a little bit. See my comments.
test/EFCore.PG.FunctionalTests/NpgsqlMigrationSqlGeneratorTest.cs
Outdated
Show resolved
Hide resolved
@@ -45,5 +44,8 @@ public static class NpgsqlAnnotationNames | |||
|
|||
[Obsolete("Replaced by ValueGenerationStrategy.SerialColumn")] | |||
public const string ValueGeneratedOnAdd = Prefix + "ValueGeneratedOnAdd"; | |||
|
|||
[Obsolete("Replaced by built-in EF Core support")] |
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.
Makes sense to write a constant in this class with a value like "Replaced by built-in EF Core support. Use HasComment"
, and reuse it for all Obsolete
attributes.
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 changed the obsolete messages to be more informative, but am keeping them separate since the ones on the builder should be much more concise/easy. The others (like this one in NpgsqlAnnotationNames) will likely not be seen by end users anyway.
Holding off on this until issues in the upstream implementation are fixed (dotnet/efcore#16819) |
Superceded by #992 |
Closes #892