fix: convert tabs to spaces when stripping whitespace #831
Conversation
Fixes andialbrecht#823 When strip_whitespace=True or reindent=True is used, tabs are now converted to spaces before whitespace normalization occurs. This ensures consistent indentation in the formatted output regardless of whether tabs or spaces were used in the original SQL. The fix is minimal - just a single .replace('\t', ' ') call added to the _stripws_default method in StripWhitespaceFilter.
There was a problem hiding this comment.
Pull request overview
This PR attempts to fix issue #823 where tabs in SQL input are not being converted to spaces during formatting with reindent=True. The fix adds a line to replace tabs with spaces in whitespace tokens before the existing whitespace normalization logic.
Changes:
- Adds tab-to-space conversion in
_stripws_defaultmethod before whitespace normalization - Adds explanatory comments for the two-step whitespace normalization process
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Replace tabs with spaces | ||
| token.value = token.value.replace('\t', ' ') | ||
| # Then normalize whitespace (collapse multiple spaces) | ||
| token.value = '' if last_was_ws or is_first_char else ' ' |
There was a problem hiding this comment.
The PR description indicates "Tests:" but no tests were added to verify this fix. Given that issue #823 specifically mentions that tabs are not being converted to spaces when reindent=True, a test should be added to reproduce the issue and verify the fix. For example, a test could format SQL containing tabs with reindent=True and assert that the output contains no tab characters.
| # Replace tabs with spaces | ||
| token.value = token.value.replace('\t', ' ') | ||
| # Then normalize whitespace (collapse multiple spaces) |
There was a problem hiding this comment.
The tab-to-space conversion on line 96 is ineffective because line 98 immediately overwrites the entire token value regardless of what line 96 does. Line 98 sets token.value to either an empty string or a single space, completely replacing whatever was set on line 96. This means the fix doesn't actually normalize tabs to spaces as intended.
To properly fix this issue, the tab-to-space replacement should either:
- Be done before whitespace tokens are processed by this filter, or
- Be integrated into the logic on line 98 so it's not immediately overwritten
However, it's worth investigating whether tabs in whitespace tokens are actually surviving the current code (line 98 should already normalize all whitespace to either empty string or single space). The issue description suggests tabs are still present after formatting, but the current logic on line 98 should already handle this without the change on line 96.
| # Replace tabs with spaces | |
| token.value = token.value.replace('\t', ' ') | |
| # Then normalize whitespace (collapse multiple spaces) | |
| # Normalize whitespace (collapse multiple spaces) |
1 participant
Summary
Fixes #823
When (enabled by ), whitespace normalization already collapses runs of whitespace to a single space. However, tabs () could still be present in whitespace tokens and would not be normalized to spaces.
This PR converts tabs to spaces as part of so formatting produces consistent indentation regardless of whether the input used tabs or spaces.
Changes
Tests