-
Notifications
You must be signed in to change notification settings - Fork 37
adds formatter option for preserving comments (redo) #187
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
base: main
Are you sure you want to change the base?
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.
Pull request overview
This PR adds a PreserveComments option to SqlScriptGeneratorOptions to enable the script generator to preserve T-SQL comments (both single-line -- and multi-line /* */) in the generated output. This addresses issue #20 which requested comment preservation for code formatting/pretty-printing scenarios.
Changes:
- Adds
PreserveCommentsboolean option toSqlScriptGeneratorOptionswith default valuefalsefor backward compatibility - Implements comment tracking and emission logic in
SqlScriptGeneratorVisitor.Comments.cs - Adds helper classes
CommentInfoandCommentPositionto manage comment metadata - Modifies
TSqlScriptandTSqlBatchvisitors to call comment emission logic before/after fragments - Adds comprehensive unit tests in
ScriptGeneratorTests.csto verify comment preservation behavior - Adds baseline test scripts
SingleLineCommentTests170.sqlandMultiLineCommentTests170.sql
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| SqlScriptGeneratorOptions.xml | Adds PreserveComments boolean setting with false default |
| SqlScriptGeneratorVisitor.Comments.cs | Core implementation of comment tracking and emission logic |
| SqlScriptGeneratorVisitor.TSqlScript.cs | Integrates comment handling at script level |
| SqlScriptGeneratorVisitor.TSqlBatch.cs | Integrates comment handling at batch and statement levels |
| CommentInfo.cs | Helper class to track comment metadata |
| CommentPosition.cs | Enum defining comment positions (Leading/Trailing/Standalone) |
| ScriptGeneratorTests.cs | 13 comprehensive unit tests for comment preservation |
| Only170SyntaxTests.cs | Adds comment test scripts to baseline test suite |
| SingleLineCommentTests170.sql | Test script with single-line comments |
| MultiLineCommentTests170.sql | Test script with multi-line comments |
| Baselines170/SingleLineCommentTests170.sql | Expected output with comments stripped (default behavior) |
| Baselines170/MultiLineCommentTests170.sql | Expected output with comments stripped (default behavior) |
SqlScriptDom/ScriptDom/SqlServer/ScriptGenerator/SqlScriptGeneratorVisitor.Comments.cs
Outdated
Show resolved
Hide resolved
SqlScriptDom/ScriptDom/SqlServer/ScriptGenerator/SqlScriptGeneratorVisitor.Comments.cs
Outdated
Show resolved
Hide resolved
| #region Comment Tracking Fields | ||
|
|
||
| /// <summary> | ||
| /// Tracks the last token index processed for comment emission. | ||
| /// Used to find comments between visited fragments. | ||
| /// </summary> | ||
| private int _lastProcessedTokenIndex = -1; | ||
|
|
||
| /// <summary> | ||
| /// The current script's token stream, set when visiting begins. | ||
| /// </summary> | ||
| private IList<TSqlParserToken> _currentTokenStream; |
Copilot
AI
Jan 29, 2026
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.
The _lastProcessedTokenIndex and _currentTokenStream fields are instance fields of SqlScriptGeneratorVisitor without any thread synchronization. If the same SqlScriptGenerator instance is used concurrently from multiple threads (which is not a typical pattern but is possible), these fields could be corrupted. Consider documenting that SqlScriptGenerator instances are not thread-safe and should not be shared across threads, or add appropriate thread-safety mechanisms.
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.
@llali curious your thoughts on thread safety for scriptgenerator. The overall overhead of a shared vs individual instances of the script generator across threads for common scenarios seems minimal so the justification doesn't seem to be there, but I'm potentially missing something.
SqlScriptDom/ScriptDom/SqlServer/ScriptGenerator/CommentPosition.cs
Outdated
Show resolved
Hide resolved
SqlScriptDom/ScriptDom/SqlServer/ScriptGenerator/SqlScriptGeneratorVisitor.Comments.cs
Outdated
Show resolved
Hide resolved
SqlScriptDom/ScriptDom/SqlServer/ScriptGenerator/SqlScriptGeneratorVisitor.TSqlBatch.cs
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.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.

Description
resolves #20
enables the code formatter to preserve T-SQL comments, including
Code Changes