-
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
Refactor comment support #16828
Refactor comment support #16828
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.
👍 In the same batch is preferable.
What does a column comment during CREATE TABLE look like?
@@ -117,6 +117,10 @@ protected override void Generate(MigrationOperation operation, IModel model, Mig | |||
MigrationCommandListBuilder builder, | |||
bool terminate) | |||
{ | |||
if (!terminate && operation.Comment != null) | |||
{ | |||
throw new ArgumentException($"When generating migrations SQL for {nameof(AddColumnOperation)}, can't produce unterminated SQL with comments"); |
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.
In SQLite, we Debug.Fail() but let it slide in Release.
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 at least here we now for sure that the SQL will come out totally mangled, no? So better to throw an informative exception?
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 mean... they could compose WITH RECOMPILE onto the last EXEC call 😜
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.
Wow, OK :) Not sure what it would mean to do that to sp_addextendedproperty... In any case it would only work on the last statement, and only if comments were provided...
Basically I can't see the non-terminating scenario being much use when multiple statements are being generated - especially when the really significant one is the first...
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'm totally fine throwing 😄 (just pointing out an existing pattern used elsewhere)
Fixes #16799
Fixes #16800
Note that I've included the comment statements (EXEC sp_addextendedproperty) in the same command as the DDL itself. Am not familiar enough with SqlServer to know if that's meaningful.