-
-
Notifications
You must be signed in to change notification settings - Fork 729
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
Fix bug where L018 warns inappropriately if CTE definition includes a column list #3227
Fix bug where L018 warns inappropriately if CTE definition includes a column list #3227
Conversation
@@ -72,4 +73,5 @@ | |||
"RegexLexer", | |||
"Parser", | |||
"Matchable", | |||
"IdentitySet", |
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.
Exposing IdentitySet
at the sqlfluff.core.parser
module level since it's useful outside of the parser (i.e. for rules).
@@ -45,4 +46,5 @@ | |||
"Indent", | |||
"Dedent", | |||
"TemplateSegment", | |||
"IdentitySet", |
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.
Exposing IdentitySet
at the sqlfluff.core.parser.segments
module level since it's useful outside of the parser (i.e. for rules).
for seg in context.segment.iter_segments( | ||
expanding=["common_table_expression", "bracketed"], pass_through=True | ||
): | ||
if seg.name == "start_bracket": |
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.
Review this section with "Hide whitespace" checked
.last(sp.is_type("bracketed")) | ||
.children() | ||
.last(sp.is_name("end_bracket"))[0] | ||
) |
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.
This code could've been simplified or avoided if CTEDefinitionSegment
had segment definitions for the optional column list and/or the query part. I thought about it, but at least one dialect besides ANSI specializes CTEDefinitionSegment
, so let's do it here instead. Something to keep in mind, though, if in future we have other rules with similar needs.
Codecov Report
@@ Coverage Diff @@
## main #3227 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 166 166
Lines 12365 12367 +2
=========================================
+ Hits 12365 12367 +2
Continue to review full report at Codecov.
|
…hub.com/barrywhart/sqlfluff into bhart-issue_2851_ignore_cte_column_list
Brief summary of the change made
Fixes #2851
Are there any other side effects of this change that we should be aware of?
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:
.yml
rule test cases intest/fixtures/rules/std_rule_cases
..sql
/.yml
parser test cases intest/fixtures/dialects
(note YML files can be auto generated withtox -e generate-fixture-yml
).test/fixtures/linter/autofix
.Added appropriate documentation for the change.
Created GitHub issues for any relevant followup/future enhancements if appropriate.