-
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
Generate Sqlite create table comments #17198
Generate Sqlite create table comments #17198
Conversation
Also, do we want to handle table-level comments? Unfortunately, CREATE TABLE "Person" (
-- Represents a person
-- The ID
"Id" INTEGER NOT NULL
) |
Hmm... another alternative I don't like: CREATE TABLE "Person" ( -- Represents a person
"Id" INTEGER NOT NULL, -- The ID
"UndocumentedColumn1" TEXT,
"UndocumentedColumn2" TEXT,
"Name" TEXT -- The name
) |
My vote: Option B; omit spaces when no columns are documented; don't preserve table-level comments. |
I think I like this the best. If there is a table comment, there should be a line after. Then each column with a comment should have a newline before the comment. CREATE TABLE "People" (
-- Table Comment
-- The ID
"Id" INTEGER NOT NULL,
"UndocumentedColumn1" TEXT,
"UndocumentedColumn2" TEXT,
-- The name
"Name" TEXT
) Do you like having a newline after the commented column better? like: CREATE TABLE "People" (
-- Table Comment
-- The ID
"Id" INTEGER NOT NULL,
"UndocumentedColumn1" TEXT,
"UndocumentedColumn2" TEXT,
-- The name
"Name" TEXT
) |
It's how I would write it, but just putting a newline before the comment is a simpler implementation, so I'd be fine with that too. |
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.
LGTM. I'll let you decide what format we should use. ¯\(ツ)/¯ Do you think we should output table-level comments? Let me know when you're ready for me to merge it.
2f53876
to
1d0a0af
Compare
src/EFCore.Sqlite.Core/Migrations/SqliteMigrationsSqlGenerator.cs
Outdated
Show resolved
Hide resolved
1d0a0af
to
da541b6
Compare
|
||
private void CreateComment(MigrationCommandListBuilder builder, string comment) | ||
{ | ||
foreach (var line in comment.Split(Environment.NewLine).Select(l => $"-- {l}")) |
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.
We’ve taken a bug fix for code that did this somewhere else. I think it was about Unix line endings on Windows or something. I’ll try to find that implementation so we can reuse it here.
@bricelam Remember to rebase on or cherry-pick to preview 9. |
/// <summary> | ||
/// The default single line comment prefix. | ||
/// </summary> | ||
protected virtual string SingleLineCommentToken { get; } = "--"; |
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.
@ajcvickers @smitpatel I'm breaking this (moving it to the general SQL generator so it can be shared with Migrations)
- This only works with create table. Alter table commands do not preserve comments. - If comments are included, each column will be spaced. - Table comments - Column comments Fixes dotnet#16820
daa5eef
to
5e79127
Compare
This only works with create table. Alter table commands do not preserve
comments.
Fixes #16820
Please check if the PR fulfills these requirements
Please review the guidelines for CONTRIBUTING.md for more details.