Add configurable SQL comment removal feature#23
Merged
Conversation
This commit introduces a comprehensive SQL comment removal system that is SQL-92 standard compliant and works across all major databases. Key Features: - Configurable comment removal strategies (:none, :oneline, :multiline, :all) - Configurable scope (:none, :prepared_for_logs, :all) - SQL standard compliant (SQL-92) - Preserves comments in quoted strings (single, double, dollar quotes) - Handles all edge cases (escaped quotes, inline comments, etc.) Implementation: - New CommentRemover service class using state machine pattern - Extracted Config class to separate file for better organization - Added Config#should_comments_be_removed? method for clean logic - Integrated into sql() and prepared_for_logs() methods Testing: - Comprehensive unit tests for CommentRemover (32 examples) - Integration tests for configuration scenarios (10 examples) - Config class tests (12 examples) - Test fixtures for various comment scenarios - 78 total tests, 98.38% code coverage - RuboCop compliant Documentation: - Updated CHANGELOG.md with feature description - Updated README.md with configuration options and examples Addresses #20
- CommentRemover now accepts config object instead of strategy symbol - CommentRemover handles all removal decisions (both strategy and scope) - Simplified SqlQuery#apply_comment_removal to always call CommentRemover - All comment removal logic now centralized in one place - Updated all tests to pass config objects and for_logs parameter - Added comprehensive tests for remove_comments_from configuration - 84 tests, all passing, 98.33% coverage - RuboCop compliant
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR introduces a comprehensive SQL comment removal system that addresses #20. The feature allows developers to configure how and when SQL comments are removed from queries, providing flexibility for different use cases (debugging, logging, production).
Key Features
Configurable removal strategies (
config.remove_comments)::none- Don't remove any comments:oneline- Remove only single-line comments (--):multiline- Remove only multi-line comments (/* */):all- Remove both types (default)Configurable scope (
config.remove_comments_from)::none- Don't remove comments anywhere:prepared_for_logs- Remove comments only inprepared_for_logsmethod:all- Remove comments from all queries (default)SQL Standard Compliance:
'-- not a comment'"-- not a comment"$$-- not a comment$$,$tag$-- not a comment$tag$Edge Case Handling:
'',"")\',\")Implementation Approach
State Machine Pattern
This implementation uses a state machine approach rather than complex regex for several reasons:
Architecture
CommentRemover Service Class (
lib/sql_query/comment_remover.rb):Config Class Extraction (
lib/sql_query/config.rb):should_comments_be_removed?(for_logs:)method for clean logicIntegration Points:
SqlQuery#sql- Applies removal based on configurationSqlQuery#prepared_for_logs- Applies removal based on configurationSqlQuery#apply_comment_removal- Helper method with clean logicTesting
Documentation
Default Behavior
By default, both comment types are removed from all queries:
Migration Path
This is a backwards-compatible addition:
config.remove_comments = :noneconfig.remove_comments_from = :prepared_for_logsTest Plan
Addresses #20