Add rule groups and a core rules group #3142
Conversation
pwildenhain
requested review from
nolanbconaway,
tunetheweb,
barrywhart and
a team
and removed request for
a team and
nolanbconaway
Codecov Report
@@ Coverage Diff @@
## main #3142 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 167 167
Lines 12423 12502 +79
=========================================
+ Hits 12423 12502 +79
Continue to review full report at Codecov.
|
I still think it might be better in the rule, for a few reasons:
|
|
Totally open to changing this approach. How should we identify when a rule group has been added to the 'rules' or 'excludes_rules' config? If it's a part of the rule, then we need to check for every rule if it's been included/excluded vs. just checking one time once the .sqlfluff config is loaded. Am I seeing that right? Or am I missing something? |
|
Yeah that would be a downside. It would presumably have to go through all rules and then check whether that rule is in the currently configured group, and skip if not. Would still be pretty quick though, but yeah maybe there's a better way to only do that once (maybe on the first query run?) and not have to do that for every SQL file. |
Yeah I agree there likely is a better way. I don't think the proposed way would work because different sql files can have different config values, if the user nests multiple |
|
Re: If the group membership was on the rules, we are already doing something similar with rule phases here: https://github.com/sqlfluff/sqlfluff/blob/main/src/sqlfluff/core/linter/linter.py#L514L516, i.e. looping over the rules, checking a property, deciding what to run. |
|
I don't see any doc changes in the PR -- were you planning to do that once there's agreement on the general approach? |
Correct -- it seems like you agree with @tunetheweb that this should be something that lives with the rules? |
|
I don't have a strong opinion where it should live, but wanted to share a note about how it could work. I may have some thoughts to share later about how we could enforce assigning a new rule somewhere, even if it's in config. |
|
The thought I mentioned earlier: If we want to be sure that we consider each new rule's membership in some group, we could add a startup check that every rule belongs to at least (or exactly one) group. Then define groups "core", "noncore", and a few others if we want. |
|
Might be better to have an |
|
Having an all group makes sense to me. Do we all agree that we want users to use the feature like this: Or is there a different vision we have for how rule groupings will be used by the user? |
|
That method of invoking it LGTM. Presume it would also work from |
|
For a moment, I was worried that using We should spend a little time thinking, documenting, and writing tests for what happens if someone mixes rules and groups in various ways, e.g.: Which wins? One possible answer: Always process |
Agree that this makes the most sense, and we should make weird test cases that will mimic what users will most certainly do in the real world |
Exactly. I don't necessarily care that they do it, but then they'll ask us to explain why it's not working, and that's hard work. 🤣 |
|
@pwildenhain: Where are we with this PR? Would be good to get it merged! Above, you mentioned you were going to make a docs change. IIRC, overall the PR was looking pretty good to me. |
|
Sure thing -- I can push the necessary changes tomorrow if we have agreement |
| if r in valid_groups: | ||
| rules_in_group = [ | ||
| rule | ||
| for rule, rule_dict in self._register.items() | ||
| if r in rule_dict["groups"] | ||
| ] |
There was a problem hiding this comment.
Maybe a question for a future PR: Does this support nested rule groups, e.g., maybe later we want to define the core group as the combination of several groups, e.g. whitespace, "making things explicit", "foobar". In that case, expansion of groups would need to be recursive.
There was a problem hiding this comment.
I could see that as a nice DRY alternative, rather than manually specifying the same rule in different groups over and over again. I agree though that we should leave it as a consideration for a future PR -- who knows, maybe no one will care about rule groups 😂
| groups = ("all", "core") | ||
| config_keywords = ["tab_space_size"] |
There was a problem hiding this comment.
Why is groups a Tuple, and config_keywords a List?
Should groups be a List?
There was a problem hiding this comment.
I made groups a tuple because I wanted to make sure we couldn't modify it after the rule is instantiated.
If we think that's no a big concern, I can turn it into a list. If we do think it's a big concern, then we should turn config_keywords into a Tuple as well for consistency
Co-authored-by: Barry Pollard <barry_pollard@hotmail.com> Co-authored-by: Barry Hart <barrywhart@yahoo.com>
Co-authored-by: Barry Pollard <barry_pollard@hotmail.com>
Brief summary of the change made
Fixes #811
As I studied the problem more, it made more sense for rule groupings to be something controlled by the config, rather than by the rule itself (like rule tags or something like that). What this change now essentially amounts to is an opinionated default list of core rules, that users can modify if they wish to include more core rules. It also makes it easier to roll out SQLFluff to a team by only running core rules at first.
Are there any other side effects of this change that we should be aware of?
User can now define their own rule groups, though I'm not sure there's much utility in that, since they still need to write out all the rule names. I think a good addition to this feature would be "common sense" names for each of the rules (i.e
L001 = "trailing-whitespace") which makes it easier for users to define things like that.It also opens up the potential for different types of groups that the package maintainers might want to offer in the future (
dbt-best-practice,safe-sql, etc.) for user convenienceIf we have agreement on this, I'll document this new feature as well as the core rule list in the docs.
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:
.ymlrule test cases intest/fixtures/rules/std_rule_cases..sql/.ymlparser test cases intest/fixtures/dialects(note YML files can be auto generated withtox -e generate-fixture-yml).test/fixtures/linter/autofix.Added appropriate documentation for the change. (Not done yet)
Created GitHub issues for any relevant followup/future enhancements if appropriate.