New Rule L052: Semi colon alignment

[jpy-git]

Brief summary of the change made

This PR introduces a new rule L052. It serves two purposes, one to snap semi-colons to the end of the preceding statement, and the other to add the final trailing semi-colon if it is missing:

I have added a configuration option called allow_final_semi_colon (which defaults to False) so users can allow missing final semi-colons if they desire (I'm happy to change the default to True on this if you would prefer, but my preference is to add the final semi-colons by default).

This change impacts quite a few unit tests, solely because we weren't previously including final semi-colon, so I have gone through and added the semi-colon/excluded the rule where I think appropriate.

Fixes #1748

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:

  • .yml rule test cases in test/fixtures/rules/std_rule_cases.
  • .sql/.yml parser test cases in test/fixtures/dialects (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.

[jpy-git]

@barrywhart took a couple attempts to track down all the places this impacted in the dbt tests, but this should be good for review now 😄

[codecov]

Codecov Report

Merging #1902 (3b2892b) into main (93c4eb3) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main     #1902   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          142       143    +1     
  Lines        10090     10152   +62     
=========================================
+ Hits         10090     10152   +62     

Impacted Files	Coverage Δ
src/sqlfluff/core/rules/config_info.py	100.00% <ø> (ø)
src/sqlfluff/rules/L052.py	100.00% <100.00%> (ø)

---

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 93c4eb3...3b2892b. Read the comment docs.

[WittierDinosaur]

I don't think this is an anti-pattern. It's fairly standard in large queries

        -- Newlines between the end of the statement
        -- and the semi-colon are forbidden.
        SELECT
            a
        FROM foo
        ;

[tunetheweb]

Personally I like this rule!

What about an optional config to also allow them to be on a newline (with no extra newlines in between) to keep the @WittierDinosaur 's of the world happy too? In which case should this be enforced or only allowed? Which should fix use if it's missing and this optional param is set?

[jpy-git]

@WittierDinosaur @tunetheweb sure thing! I don't think it should be too hard to add that option.

The way it will work is user has an option of either semi-colon on same line OR new line, but not both. Consistency should be encouraged.

The default will be to use same line.

[alanmcruickshank]

So - potential spanner in the works for this rule. Trailing semi colons are explicitly a problem for dbt users:

Reason being that often queries are "wrapped" by other sql and so the trailing semicolon becomes an issue.

Potential solutions:

• This rule is disabled by default for users when using the dbt templater (although I'm not sure exactly what the mechanism should be to do that).
• That semi-colons between statements are linted by default, but that the default is that trailing semicolons aren't added.

I think I prefer the latter, that this rule:

• moves any semi-colons present.
• optionally adds trailing semi-colons if not present, but this is not enabled by default.

[jpy-git]

@alanmcruickshank ah was not aware of that, agree that your approach to disable the trailing semi-colon by default is pragmatic though, will tweak the default config 👍

[alanmcruickshank]

@alanmcruickshank ah was not aware of that, agree that your approach to disable the trailing semi-colon by default is pragmatic though, will tweak the default config 👍

If we're taking that option - would you mind removing the semi-colons from the tests in the dbt tests. I'm happy for them to stay on the core tests - but I think that sets the right expectation for performance with dbt. Bonus points if you make a note in the default config to that effect.

[jpy-git]

Will do! 😄

[jpy-git]

Addressed the "allow_final_semi_colon" being set to true by default point. Will move on to looking at the option for semi-colons on a new line now

[jpy-git]

@WittierDinosaur @tunetheweb @alanmcruickshank I've added the options to configure the behaviour of rule L052 as we described above. I've also added extra unit tests for the config options.

Demo:

Should be good to go if you're happy with it 😄

[jpy-git]

@barrywhart Ready for review 😄

[barrywhart]

Tagging other commenters for review. I'll take a look as well.

[tunetheweb]

LGTM

[jpy-git]

@tunetheweb @barrywhart think we can merge this, L053, and L054 down once the final CI checks are done, I've addressed all the suggestions and test cases look good