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

L052: Refactor _eval() into individual functions to improve readability #2733

Conversation

barrywhart
Copy link
Member

@barrywhart barrywhart commented Feb 27, 2022

Brief summary of the change made

This PR is a straightforward but thorough reorganization of L052, splitting the long, complex _eval() function into several smaller, easier to understand functions. Benefits:

  • Makes future work on L052 easier
  • Future potential for reusing some of this code in other rules. Other rules sometimes need to move a segment to a different line. L018 is one example. (See issue L018 (WITH clause closing bracket should be aligned with WITH keyword): Add support for moving ")" down to the next line #2713.) Currently, L018 is less sophisticated than L052 -- it basically just adds a newline, splitting one line into two. L052 is more sophisticated: If there is a trailing comment on the line, it leaves the comment on the same line, moving only the semicolon to the next line. This is tricky enough that we probably wouldn't want to write this code twice.

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/dialects (note YML files can be auto generated with tox -e generate-fixture-yml).
    • 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.

@codecov
Copy link

codecov bot commented Feb 27, 2022

Codecov Report

Merging #2733 (85a5e11) into main (ce9cd4a) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##              main     #2733   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          163       163           
  Lines        12061     12082   +21     
=========================================
+ Hits         12061     12082   +21     
Impacted Files Coverage Δ
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 ce9cd4a...85a5e11. Read the comment docs.

@tunetheweb tunetheweb merged commit 0778b47 into sqlfluff:main Feb 28, 2022
@tunetheweb tunetheweb changed the title L052: Refactor _eval() into individual functions to improve readability L052: Refactor _eval() into individual functions to improve readability Mar 6, 2022
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

2 participants