Prevent exceptions when running fix on dialect fixtures
#2818
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2818 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 163 163
Lines 12322 12326 +4
=========================================
+ Hits 12322 12326 +4
Continue to review full report at Codecov.
|
tunetheweb
changed the title
fix on dialect fixtures
|
can we add test cases for the things it fixes? |
Yeah probably should. This started out as one simple fix, but growing legs as I scan the rest of the dialect fixtures. Will start to add some test cases. |
Added where possible. Through two I couldn't get to work under pytest. |
| @@ -802,28 +802,32 @@ def _choose_anchor_segment( | |||
|
|
|||
| anchor: BaseSegment = segment | |||
| child: BaseSegment = segment | |||
| for seg in context.parent_stack[0].path_to(segment)[1:-1][::-1]: | |||
There was a problem hiding this comment.
Review this bit hiding whitespace:
| assert len(table_aliases) > 0, "Only Selects with tables should be checked" | ||
| if len(table_aliases) > 1: | ||
| if len(table_aliases) != 1: | ||
| return None |
There was a problem hiding this comment.
Don't see why we abort on more than one, but not on 0? Same thing IMHO and this fixes one of the issues raised.
|
This should be good to go now. Includes the See note about, about timing though :-( But only a minute or so slower and I think worth it. |
| for DIALECT in "${DIALECTS[@]}" | ||
| do | ||
| echo "Testing $DIALECT SQL files fix without critical errors..." | ||
| OUTPUT=$(sqlfluff fix -f ${DIALECT}) |
There was a problem hiding this comment.
Interested in adding other .sqlfluff files, e.g. Danny's from #2811? (Feel free to create a ticket for later.)
There was a problem hiding this comment.
It's already the longest running job so would suggest a separate, parallel, job if we do that.
However I did test it with tab_space = 2 and got no CRITICAL errors (which is all it currently looks for) any more since your fix, plus the ones here.
Are you still getting the errors if you do it on this branch? Cause if not, then no need to add. The default config identified the issues and they were fixed.
tunetheweb
deleted the
prevent-itteration-error-in-get_select_statement_info
branch
Brief summary of the change made
Fixes #2817 with simple additional safeguard test.
Fixes #2812
Also adds a few more I've found in similar cases.
And updates our CI to test
fixinstead oflintagainst dialect fixtures (notfixdoes alintas part of that solintis still tested), to prevent similar future issues.Are there any other side effects of this change that we should be aware of?
The
fixtests take longer to run, finishing just after windows tests (which was previous slowest test) so CI does slow down a couple of mins :-(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 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.