Prevent L019 plus L034 corrupting SQL #1803
Conversation
| # Reuse the previous leading comma violation to | ||
| # create a new trailing comma | ||
| [last_code_seg, last_leading_comma_seg], | ||
| "create", |
There was a problem hiding this comment.
This is the fix. If we "edit", it corrupts the parse tree by making the newly placed comma segment a child of the code segment. This is wrong. It should be a sibling. We achieve this by using "create" instead. This requires using a different anchor: the segment following the last code segment, not the last code segment itself.
This bug is a bit similar to the #1304 bug from a month or so ago where lint fixes end up placing whitespace segments in weird places they don't belong. Noting this because there's an open question whether the core linter could help detect or avoid such issues. I suggest we continue fixing the individual rules while we can, but this deeper improvement may prove necessary at some point.
| @@ -67,7 +66,7 @@ def _last_comment_seg(raw_stack): | |||
| return None | |||
|
|
|||
| @staticmethod | |||
| def _last_code_seg(raw_stack): | |||
| def _last_code_seg(raw_stack: Tuple[RawSegment, ...]) -> Optional[RawSegment]: | |||
There was a problem hiding this comment.
Just adding type hints; no change in functionality.
| @@ -77,6 +76,17 @@ def _last_code_seg(raw_stack): | |||
| return segment | |||
| return None | |||
|
|
|||
| @staticmethod | |||
| @@ -0,0 +1,5 @@ | |||
| SELECT | |||
There was a problem hiding this comment.
Previously, the bug was corrupting the SQL, leaving commas in the wrong place.
| @@ -123,7 +133,7 @@ def _eval(self, context: RuleContext) -> Optional[LintResult]: | |||
| # A comma preceded by a new line == a leading comma | |||
| if context.segment.is_type("comma"): | |||
| last_seg = self._last_code_seg(context.raw_stack) | |||
| if last_seg.is_type("newline"): | |||
| if last_seg and last_seg.is_type("newline"): | |||
There was a problem hiding this comment.
Checking that last_seg is not None. mypy wanted this, and this may have fixed a bug (I didn't study closely).
Codecov Report
@@ Coverage Diff @@
## main #1803 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 138 138
Lines 9800 9819 +19
=========================================
+ Hits 9800 9819 +19
Continue to review full report at Codecov.
|
src/sqlfluff/rules/L019.py
Outdated
| @@ -77,6 +76,16 @@ def _last_code_seg(raw_stack): | |||
| return segment | |||
| return None | |||
|
|
|||
| @staticmethod | |||
| def _follows_seg( | |||
There was a problem hiding this comment.
Not loving the name here. I'm not a Python developer but is there a better name? Like _get_following_seg or _get_next_seg?
There was a problem hiding this comment.
👍 I'll change it to _get_following_seg.
…//github.com/barrywhart/sqlfluff into bhart-issue_1734_l019_plus_l034_corrupts_sql
tunetheweb
changed the title
tunetheweb
changed the title
Brief summary of the change made
Fixes #1734
Are there any other side effects of this change that we should be aware of?
No.
Pull Request checklist
Please confirm you have completed any of the necessary steps below.
Included test cases to demonstrate any code changes, which may be one or more of the following:
.ymlrule test cases intest/fixtures/rules/std_rule_cases..sql/.ymlparser test cases intest/fixtures/dialects(note YML files can be auto generated withpython test/generate_parse_fixture_yml.pyor by runningtoxlocally).test/fixtures/linter/autofix.Added appropriate documentation for the change.
Created GitHub issues for any relevant followup/future enhancements if appropriate.