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

Layout Rules Recode (part 2) #4456

Merged
merged 11 commits into from Mar 5, 2023
Merged

Conversation

alanmcruickshank
Copy link
Member

@alanmcruickshank alanmcruickshank commented Mar 3, 2023

This is a follow on from #4432 . It's part of #4031 .

This PR is primarily about consolidating all the other spacing rules into LT01. This removes a lot of individual rules, but results in much less code to basically do the same thing. We rely on layout configs to allow users to tweek formats for their setup. Specifically, LT01 now combines:

  • L001: Trailing Whitespace
  • L005 & L008: Space around commas
  • L006: Space around operators
  • L023: Space after AS in WITH clause
  • L024: Space immediately after USING
  • L039: Unnecessary Whitespace
  • L048: Spacing around quoted literals
  • L071: Spacing around brackets

In the process this also revises a couple of spacing configurations. In particular I've changed inline to be a qualifier so that we can have single:inline and touch:inline (where the latter was the previous meaning of just inline). This also allows a little more modularity in the code which handles it.

In the original issue - I suggested that we should also combine L015 and L017 into here too, but on closer inspection I think that's a bad idea. I'll handle them separately.

post_constraint = next_block.spacing_before if next_block else "single"
# Unless align, split.
if post_constraint.startswith("align"):
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you not just loop through the constraints rather than repeating the code block?

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah that's probably a better idea... 🤔

@coveralls
Copy link

Pull Request Test Coverage Report for Build 4331471805

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at ?%

Totals Coverage Status
Change from base Build 4331251036: 0%
Covered Lines:
Relevant Lines: 0

💛 - Coveralls

@coveralls
Copy link

coveralls commented Mar 4, 2023

Pull Request Test Coverage Report for Build 4336164845

  • 27 of 27 (100.0%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 4331650673: 0.0%
Covered Lines: 17245
Relevant Lines: 17245

💛 - Coveralls

@codecov
Copy link

codecov bot commented Mar 4, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change

Comparison is base (befc52d) 100.00% compared to head (db1afbf) 100.00%.

Additional details and impacted files
@@            Coverage Diff            @@
##              main     #4456   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          211       204    -7     
  Lines        15498     15415   -83     
=========================================
- Hits         15498     15415   -83     
Impacted Files Coverage Δ
src/sqlfluff/core/rules/crawlers.py 100.00% <ø> (ø)
src/sqlfluff/dialects/dialect_sparksql.py 100.00% <ø> (ø)
src/sqlfluff/rules/layout/LT01.py 100.00% <100.00%> (ø)
src/sqlfluff/utils/reflow/respace.py 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@alanmcruickshank alanmcruickshank changed the title [draft] Layout Rules Recode (part 2) Layout Rules Recode (part 2) Mar 5, 2023
@alanmcruickshank alanmcruickshank marked this pull request as ready for review March 5, 2023 12:32
Copy link
Contributor

@WittierDinosaur WittierDinosaur left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -291,7 +291,7 @@ above:
.. code-block:: cfg

[sqlfluff]
warnings = L019, L006
warnings = L019, LT01
Copy link
Member

Choose a reason for hiding this comment

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

Oh interesting, I didn't know we were downgrading this to a warning

Copy link
Member Author

Choose a reason for hiding this comment

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

... we're not - it's just an example 😄

Copy link
Member

Choose a reason for hiding this comment

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

🙃

@alanmcruickshank alanmcruickshank merged commit ce57b75 into main Mar 5, 2023
@alanmcruickshank alanmcruickshank deleted the ac/rules_reorg_more_spacing branch March 5, 2023 16:37
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.

None yet

4 participants