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

Allow L059 to be configured to always prefer quoted identifiers #2537

Merged
merged 36 commits into from
Feb 2, 2022

Conversation

niconoe-
Copy link
Contributor

@niconoe- niconoe- commented Feb 1, 2022

Brief summary of the change made

fixes #2527 by providing a configuration option on the L059 rule.

Are there any other side effects of this change that we should be aware of?

No side effects expected as the default values for the newly added configuration are respecting the previous L059 behavior.

Pull Request checklist

  • Included test cases to demonstrate any code changes: .yml rule test cases in test/fixtures/rules/std_rule_cases.
  • Added appropriate documentation for the change.
  • Created GitHub issues for any relevant followup/future enhancements if appropriate.

Even though I think I have fulfilled the requirements in terms of development, tests, documentation, please do not hesitate to help me ensuring this PR is OK, as I didn't wrote anything in python since a decade (literally), and I'm not 100% sure I respected the code of conduct (and if I failed, I'm sorry, I'll fix my mistakes).

Extend L059 to allow user-defined configuration to set if the identifiers must be always quoted or preferably unquoted, and when quotes are necessary or wanted, which one to use.
Add new configurations for L059
Manage user-defined configuration to allow to force quoted identifiers, and prefer some quotes over others.

See sqlfluff#2527
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.

Very good work! However I do have quite a few comments and fear this won't work for some dialects as is.

src/sqlfluff/core/default_config.cfg Outdated Show resolved Hide resolved
src/sqlfluff/core/rules/config_info.py Outdated Show resolved Hide resolved
src/sqlfluff/core/rules/config_info.py Outdated Show resolved Hide resolved
src/sqlfluff/rules/L059.py Outdated Show resolved Hide resolved
src/sqlfluff/rules/L059.py Outdated Show resolved Hide resolved
src/sqlfluff/rules/L059.py Outdated Show resolved Hide resolved
src/sqlfluff/rules/L059.py Outdated Show resolved Hide resolved
src/sqlfluff/rules/L059.py Outdated Show resolved Hide resolved
src/sqlfluff/rules/L059.py Outdated Show resolved Hide resolved
test/fixtures/rules/std_rule_cases/L059.yml Outdated Show resolved Hide resolved
niconoe- and others added 8 commits February 1, 2022 14:53
Co-authored-by: Barry Pollard <barry_pollard@hotmail.com>
Co-authored-by: Barry Pollard <barry_pollard@hotmail.com>
Co-authored-by: Barry Pollard <barry_pollard@hotmail.com>
Co-authored-by: Barry Pollard <barry_pollard@hotmail.com>
@niconoe-
Copy link
Contributor Author

niconoe- commented Feb 1, 2022

@tunetheweb
I'll work tomorrow on renaming preferred_quote_identifier by force_identifier_quote_type and allow blank, default value to blank, to be as close as possible from the behavior you described to me today.
I also have seen lots of tests and lints aren't passing, so I'll give it a look tomorrow as well.

Thanks a lot for your participation

Fix tests as default dialect is ansi and backticks are not parsed. Added bigquery dialect tests to ensure this is working also with backticks when dialect is supporting them.
src/sqlfluff/rules/L059.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Feb 2, 2022

Codecov Report

Merging #2537 (958e322) into main (b1facf1) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##              main     #2537   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          163       163           
  Lines        11758     11768   +10     
=========================================
+ Hits         11758     11768   +10     
Impacted Files Coverage Δ
src/sqlfluff/core/rules/config_info.py 100.00% <ø> (ø)
src/sqlfluff/rules/L059.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 b1facf1...958e322. Read the comment docs.

src/sqlfluff/core/default_config.cfg Outdated Show resolved Hide resolved
src/sqlfluff/core/rules/config_info.py Outdated Show resolved Hide resolved
src/sqlfluff/rules/L059.py Outdated Show resolved Hide resolved
src/sqlfluff/rules/L059.py Outdated Show resolved Hide resolved
src/sqlfluff/rules/L059.py Outdated Show resolved Hide resolved
src/sqlfluff/rules/L059.py Outdated Show resolved Hide resolved
src/sqlfluff/rules/L059.py Outdated Show resolved Hide resolved
src/sqlfluff/rules/L059.py Outdated Show resolved Hide resolved
test/fixtures/rules/std_rule_cases/L059.yml Outdated Show resolved Hide resolved
niconoe- and others added 8 commits February 2, 2022 17:03
Co-authored-by: Barry Pollard <barry_pollard@hotmail.com>
Co-authored-by: Barry Pollard <barry_pollard@hotmail.com>
Co-authored-by: Barry Pollard <barry_pollard@hotmail.com>
Co-authored-by: Barry Pollard <barry_pollard@hotmail.com>
Co-authored-by: Barry Pollard <barry_pollard@hotmail.com>
Co-authored-by: Barry Pollard <barry_pollard@hotmail.com>
Co-authored-by: Barry Pollard <barry_pollard@hotmail.com>
@niconoe- niconoe- closed this Feb 2, 2022
@niconoe- niconoe- reopened this Feb 2, 2022
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.

Pretty much there. One suggestion that I'll commit and then merge.

test/fixtures/rules/std_rule_cases/L059.yml Outdated Show resolved Hide resolved
@tunetheweb tunetheweb changed the title User-defined configuration for L059 Allow L059 to be configured to always prefer quoted identifiers Feb 2, 2022
@tunetheweb tunetheweb merged commit 0102303 into sqlfluff:main Feb 2, 2022
@tunetheweb
Copy link
Member

Thanks for working on this @niconoe- ! And for being patient with all the feedback.

@niconoe- niconoe- deleted the patch-1 branch February 2, 2022 17:36
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.

L059: Allow prefered quoted for identifiers
2 participants