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

Lint spaces in qualified names #2130

Merged
merged 13 commits into from
Dec 19, 2021

Conversation

jpers36
Copy link
Contributor

@jpers36 jpers36 commented Dec 17, 2021

Fixes #1754

This rule change adds another type of unnecessary whitespace to L039. Any whitespace or newline segments found within a reference segment type are removed.

Per @jpy-git 's suggestion this has been implemented as a change to an existing rule, but this could be spun out into its own rule if that's preferred.

Anti-pattern:

SELECT [thistable] . [col]
FROM [thisdatabase] .     [thisschema]
   . [thistable]

Pattern:

SELECT [thistable].[col]
FROM [thisdatabase].[thisschema].[thistable]

@codecov
Copy link

codecov bot commented Dec 17, 2021

Codecov Report

Merging #2130 (117c647) into main (e63a17d) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##              main     #2130   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          148       148           
  Lines        10667     10674    +7     
=========================================
+ Hits         10667     10674    +7     
Impacted Files Coverage Δ
src/sqlfluff/rules/L039.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 e63a17d...117c647. Read the comment docs.

@jpers36 jpers36 marked this pull request as ready for review December 17, 2021 20:35
Copy link
Contributor

@jpy-git jpy-git left a comment

Choose a reason for hiding this comment

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

One thing that #2137 highlighted is that the changes to this rule are triggering warnings in some situations.

For example, in the unit test in test/rules/std_L003_L036_L039_combo_test.py if you try fixing the query you'll see these warnings:
image

As far as I can see there's nothing in this that should be touched by your addition but it is. Can you investigate please?

query here:

WITH example AS (
        SELECT my_id,
            other_thing,
            one_more
        FROM
            my_table
    )

    SELECT *
    FROM example

src/sqlfluff/rules/L039.py Outdated Show resolved Hide resolved
src/sqlfluff/rules/L039.py Outdated Show resolved Hide resolved
src/sqlfluff/rules/L039.py Outdated Show resolved Hide resolved
jpers36 and others added 5 commits December 19, 2021 07:39
Co-authored-by: Joseph Young <80432516+jpy-git@users.noreply.github.com>
Co-authored-by: Joseph Young <80432516+jpy-git@users.noreply.github.com>
Co-authored-by: Joseph Young <80432516+jpy-git@users.noreply.github.com>
@jpers36
Copy link
Contributor Author

jpers36 commented Dec 19, 2021

@jpy-git I looked into it just now, and it's due to #1304 as well. The real fix would be to get that issue fixed. The workaround would be to extend the workaround I added so that it ignores multiple leading whitespace segments and not just the first segment. It shouldn't be hard, but the first solutions that come to mind are all inelegant.

@jpers36 jpers36 requested a review from jpy-git December 19, 2021 14:54
Copy link
Contributor

@jpy-git jpy-git left a comment

Choose a reason for hiding this comment

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

@jpers36 Looks good and your suggestion to handle the leading whitespace seems pragmatic 👍

Re: the incorrect parse tree issue causing those leading whitespaces in the object reference, we're well aware of the issue and it's a fairly complicated one. I'm doing some initial investigation atm. We can come back and tidy up the extra logic here in the future once that's fixed 😄

Nice work, LGTM! 🚀

@jpy-git jpy-git merged commit f41c5d1 into sqlfluff:main Dec 19, 2021
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.

New Rule suggestion: flag spaces in qualified names
2 participants