Bug fix: L016 ("Line is too long") should consider length of prior fixes #2587
Conversation
src/sqlfluff/rules/L016.py
Outdated
| @@ -443,7 +443,7 @@ def _compute_source_length(cls, segments: Sequence[BaseSegment]) -> int: | |||
| # source slice, and if we didn't correct for this, we'd count the | |||
| # length of {{bi_ecommerce_orders}} roughly 10 times, resulting in | |||
| # vast overcount of the source length. | |||
| if slice not in seen_slices: | |||
| if not segment.is_templated or slice not in seen_slices: | |||
There was a problem hiding this comment.
See the pre-existing block of comments above to understand the intent of this code. It turns out that this code was accidentally ignoring new segments (introduced by earlier fixes). By definition, these new segments cannot be templated (because SQLFluff itself introduced them during lint/fix, i.e. after templating).
There was a problem hiding this comment.
Struggling to get my head round this, but isn't there a concern this will include any untemplated code that has been seen?
What's the precedence of not here?
Is this:
if not (segment.is_templated or slice not in seen_slices):or
if (not segment.is_templated) or (slice not in seen_slices):
Codecov Report
@@ Coverage Diff @@
## main #2587 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 163 163
Lines 11876 11876
=========================================
Hits 11876 11876
Continue to review full report at Codecov.
|
| if ( | ||
| slice[0] == slice[1] and not segment.is_meta | ||
| ) or slice not in seen_slices: |
There was a problem hiding this comment.
This is much nicer! And easier to understand.
Do wonder if there's any other zero-length segments like meta? Are comments?
There was a problem hiding this comment.
Not sure. Note that if they're 0-length, it's safe to process them anyway. I added is_meta more to clarify the intent. It works fine either way.
There was a problem hiding this comment.
I checked a comment in the debugger. Length works as expected (not 0).
(Pdb) this_line[-1]
<CommentSegment: ([L: 6, P: 1]) "-- The following is the slope of the regression line. Note that CORR (which is the Pearson's correlation">
(Pdb) this_line[-1].raw
"-- The following is the slope of the regression line. Note that CORR (which is the Pearson's correlation"
(Pdb) self._compute_segment_length(this_line[-1])
104
| # segment case described above because new segments are never templated | ||
| # (because "sqlfluff fix" produced them, not the templater!). | ||
| if ( | ||
| slice[0] == slice[1] and not segment.is_meta |
There was a problem hiding this comment.
@tunetheweb: This is a slightly different way of implementing a similar check. I used the Python debugger to run these two L016 test cases and that none of the (templated) segments pass this check.
test_compute_line_length_before_template_expansion_1:
# Line 3 is fine before expansion. Too long after expansion is NOT considered
# a violation.
pass_str: |
SELECT user_id
FROM
`{{bi_ecommerce_orders}}` {{table_at_job_start}}
configs:
core:
dialect: bigquery
templater:
jinja:
context:
table_at_job_start: FOR SYSTEM_TIME AS OF CAST('2021-03-02T01:22:59+00:00' AS TIMESTAMP)
bi_ecommerce_orders: bq-business-intelligence.user.ecommerce_orders
test_compute_line_length_before_template_expansion_2:
# Line 3 is too long before expansion. It's fine after expansion, but the rule
# does not look at that.
fail_str: |
SELECT user_id
FROM
`{{bi_ecommerce_orders_bi_ecommerce_orders}}` AS {{table_alias_table_alias_table_alias_table_alias_table_alias_table_alias}}
configs:
core:
dialect: bigquery
templater:
jinja:
context:
bi_ecommerce_orders_bi_ecommerce_orders: bq-business-intelligence.user.ecommerce_orders
table_alias_table_alias_table_alias_table_alias_table_alias_table_alias: t
…ectly_after_linter_pass_1
…ectly_after_linter_pass_1
Brief summary of the change made
Fixes #2584
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:
.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.