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

CLI/.sqlfluff enhancement: Rule globs #1972

Merged
merged 9 commits into from Nov 27, 2021
Merged

CLI/.sqlfluff enhancement: Rule globs #1972

merged 9 commits into from Nov 27, 2021

Conversation

jpy-git
Copy link
Contributor

@jpy-git jpy-git commented Nov 24, 2021

Brief summary of the change made

Fixes #1445. This PR provides the mechanism for rules to be included/excluded by globs in either the config files or via the CLI.
image

As well as resolving the linked issue, the big benefit of this will be to allow users to more easily bulk disable groups of rules
via the rule prefix, as I discussed in #1812. This is essentially what is happening in the #1445 as well just for plugins. Personally think this is a very elegant solution. Also this use case is what motivates the use of globs rather than regex as the expected use is simply --rules "Core_*" or --exclude-rules "Some_Plugin_*" (or whatever prefixes we decide on), so keeps things nice and simple.

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.

@codecov
Copy link

codecov bot commented Nov 24, 2021

Codecov Report

Merging #1972 (529dec9) into main (2b7d79e) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##              main     #1972   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          148       148           
  Lines        10311     10319    +8     
=========================================
+ Hits         10311     10319    +8     
Impacted Files Coverage Δ
src/sqlfluff/core/rules/base.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 2b7d79e...529dec9. Read the comment docs.

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.

I've a few questions:

  1. Does this allow exclude_rules = L001, L05* and if so should we add a test?
  2. Should we update our documentation for this?
  3. Should we take this as an opportunity to move to the more inclusive "allowlist/denylist" syntax? At least for the code touched here?

@jpy-git
Copy link
Contributor Author

jpy-git commented Nov 27, 2021

  1. Yes it does, will add that unit test in!
  2. Sounds like a plan. I think it makes sense to address this as part of what we discussed in How to disable a rule via the config? #1938. So let's make it a separate PR and address both at the same time.
  3. Agree that would be a nice change 😄 Let me assess how much much the terms are used and then can replace them all at once (may be worth separate PR if there are loads)

An additional question I'm wondering myself is if this glob stuff also applies to inline comments. I think it does but let's resolve #1985 first and then I'll investigate 👍

@barrywhart
Copy link
Member

Looks like a dbt test failed in one of the builds?

@jpy-git
Copy link
Contributor Author

jpy-git commented Nov 27, 2021

@barrywhart it didn't fail, the GHA runner lost internet connection so the install download failed. Should work fine once re-run 👍

@jpy-git
Copy link
Contributor Author

jpy-git commented Nov 27, 2021

@tunetheweb Follow up on your questions

    1. I've added the extra unit test
    1. will be separate pr
    1. I've had a look and will make a separate PR for this otherwise the diffs will look a mess 👍
    1. These globs don't apply to inline comments at the moment because the inline comment code has no interaction with the rule set and just checks if one of the supplied rules matches an error raised. Will make a follow up PR to extend this 👍

LGTM 🚀

@tunetheweb tunetheweb merged commit 93d8955 into sqlfluff:main Nov 27, 2021
@jpy-git jpy-git deleted the rule_globs branch November 27, 2021 16:58
@jpy-git jpy-git changed the title Rule globs CLI/.sqlfluff enhancement: Rule globs Dec 11, 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.

fix CLI - exclude rules by pattern
3 participants