Skip to content
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

Issue 845: L016 should compute line length prior to template expansion #1411

Conversation

barrywhart
Copy link
Member

@barrywhart barrywhart commented Sep 15, 2021

Fixes #845
...

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 in test/fixtures/rules/std_rule_cases.
    • .sql/.yml parser test cases in test/fixtures/parser (note YML files can be auto generated with python test/generate_parse_fixture_yml.py or by running tox locally).
    • Full autofix test cases in test/fixtures/linter/autofix.
    • Other.
  • Added appropriate documentation for the change.

  • Created GitHub issues for any relevant followup/future enhancements if appropriate.

@barrywhart barrywhart marked this pull request as draft September 15, 2021 20:08
@barrywhart barrywhart marked this pull request as ready for review September 15, 2021 21:02
@barrywhart barrywhart marked this pull request as draft September 15, 2021 21:12
@barrywhart barrywhart marked this pull request as ready for review September 15, 2021 21:16
@codecov
Copy link

codecov bot commented Sep 15, 2021

Codecov Report

Merging #1411 (5331476) into main (178e2a1) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##              main     #1411   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          127       127           
  Lines         8553      8580   +27     
=========================================
+ Hits          8553      8580   +27     
Impacted Files Coverage Δ
src/sqlfluff/core/rules/config_info.py 100.00% <ø> (ø)
src/sqlfluff/api/info.py 100.00% <100.00%> (ø)
src/sqlfluff/cli/commands.py 100.00% <100.00%> (ø)
src/sqlfluff/core/dialects/common.py 100.00% <100.00%> (ø)
src/sqlfluff/core/linter/common.py 100.00% <100.00%> (ø)
src/sqlfluff/core/linter/linted_dir.py 100.00% <100.00%> (ø)
src/sqlfluff/core/linter/linted_file.py 100.00% <100.00%> (ø)
src/sqlfluff/core/linter/linter.py 100.00% <100.00%> (ø)
src/sqlfluff/core/linter/linting_result.py 100.00% <100.00%> (ø)
src/sqlfluff/core/parser/grammar/conditional.py 100.00% <100.00%> (ø)
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update defb174...5331476. Read the comment docs.

{{corr_states}}
{% for action in considered_actions %}
, aaa(
bbb(ccc({{metric}}_r, {{action}}), ddd({{metric}}_r)),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test specifies max_line_length = 50, but this line was 67 characters long. The new result looks correct to me.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A reverse case of what you are trying to fix here!

Post templating this is 47-48 characters long as the template values where shorter than the template place holders.

So I agree it's correct that it now splits it with your code change as it's now looking at the placeholder lengths.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly. The previous behavior was incorrect, and often very confusing from a user perspective.

Copy link
Member

@tunetheweb tunetheweb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. A few comments for you to consider.

src/sqlfluff/rules/L016.py Show resolved Hide resolved
src/sqlfluff/rules/L016.py Outdated Show resolved Hide resolved
src/sqlfluff/rules/L016.py Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

L016 reports "Line is too long" AFTER template expansion
2 participants