Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 32 additions & 1 deletion src/dbt_score/cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,12 @@ def cli() -> None:
default=None,
multiple=True,
)
@click.option(
"--selected-rule",
help="Rule to select.",
default=None,
multiple=True,
)
@click.option(
"--manifest",
"-m",
Expand Down Expand Up @@ -121,6 +127,7 @@ def lint( # noqa: PLR0912, PLR0913, C901
select: tuple[str],
namespace: list[str],
disabled_rule: list[str],
selected_rule: list[str],
manifest: Path,
run_dbt_parse: bool,
fail_project_under: float,
Expand All @@ -136,12 +143,19 @@ def lint( # noqa: PLR0912, PLR0913, C901
if manifest_provided and run_dbt_parse:
raise click.UsageError("--run-dbt-parse cannot be used with --manifest.")

if len(selected_rule) > 0 and len(disabled_rule) > 0:
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice 👌

We also need to take into account pyproject.toml configuration I think.

Theoretically I could...

[tool.dbt-score]
disabled_rules = ["dbt_score.rules.generic.columns_have_description"]
selected_rules = ["dbt_score.rules.generic.model_has_description"]

OR

[tool.dbt-score]
disabled_rules = ["dbt_score.rules.generic.columns_have_description"]

dbt-score lint --selected-rule dbt_score.rules.generic.model_has_description

Which should not be possible IMO. So maybe we should add more validation to Config.validate()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just catching up, it's been busy and lost track of this! I think your second example in particular feels like something I'd like to consider further. I could see utility in being able to say, effectively, dbt_score.rules.generic.columns_have_description is never run by default but there may be a certain scenario in which I explicitly want to run it, and it'd be beneficial to be able to do so at runtime.

Is it possible to override the TOML-level configs for disabled_rules from the command line? If dbt_score.rules.generic.columns_have_description is disabled per the TOML, can I un-disable it via the CLI in some way? It looks like I can potentially replace the list of rules that is disabled at runtime, but can I remove it?

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe that disabling 1 or more rule via the CLI (--disabled-rule foo --disabled-rule bar) will effectively replace the set of rules disabled via the toml config file.
Meaning you can remove at runtime the disabled rules from the config file, but only if you disable at least another rule 😅 Which is of course not great.

Not sure what the best approach is there. An example could be changing the CLI parameter to accept lists, including empty lists, e.g. --disabled-rules foo bar or --disabled-rules (no value = empty list).

raise click.UsageError(
"--disabled-rule and --selected-rule cannot be used together."
)

config = Config()
config.load()
if namespace:
config.overload({"rule_namespaces": namespace})
if disabled_rule:
config.overload({"disabled_rules": disabled_rule})
if selected_rule:
config.overload({"selected_rules": selected_rule})
if fail_project_under:
config.overload({"fail_project_under": fail_project_under})
if fail_any_item_under:
Expand Down Expand Up @@ -194,6 +208,12 @@ def lint( # noqa: PLR0912, PLR0913, C901
default=None,
multiple=True,
)
@click.option(
"--selected-rule",
help="Rule to select.",
default=None,
multiple=True,
)
@click.option(
"--title",
help="Page title (Markdown only).",
Expand All @@ -207,14 +227,25 @@ def lint( # noqa: PLR0912, PLR0913, C901
default="terminal",
)
def list_command(
namespace: list[str], disabled_rule: list[str], title: str, format: str
namespace: list[str],
disabled_rule: list[str],
selected_rule: list[str],
title: str,
format: str,
) -> None:
"""Display rules list."""
if len(selected_rule) > 0 and len(disabled_rule) > 0:
raise click.UsageError(
"--disabled-rule and --selected-rule cannot be used together."
)

config = Config()
config.load()
if namespace:
config.overload({"rule_namespaces": namespace})
if disabled_rule:
config.overload({"disabled_rules": disabled_rule})
if selected_rule:
config.overload({"selected_rules": selected_rule})

display_catalog(config, title, format)
2 changes: 2 additions & 0 deletions src/dbt_score/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ class Config:
_options: Final[list[str]] = [
"rule_namespaces",
"disabled_rules",
"selected_rules",
"inject_cwd_in_python_path",
"fail_project_under",
"fail_any_item_under",
Expand All @@ -67,6 +68,7 @@ def __init__(self) -> None:
"""Initialize the Config object."""
self.rule_namespaces: list[str] = ["dbt_score.rules", "dbt_score_rules"]
self.disabled_rules: list[str] = []
self.selected_rules: list[str] = []
self.inject_cwd_in_python_path = True
self.rules_config: dict[str, RuleConfig] = {}
self.config_file: Path | None = None
Expand Down
8 changes: 7 additions & 1 deletion src/dbt_score/rule_registry.py
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,13 @@ def _add_rule(self, rule: Type[Rule]) -> None:
rule_name = rule.source()
if rule_name in self._rules:
raise DuplicatedRuleException(rule_name)
if rule_name not in self.config.disabled_rules:
if (
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add a small comment that explains this? Our future selves will be grateful 😂

len(self.config.selected_rules) > 0
and rule_name in self.config.selected_rules
) or (
len(self.config.selected_rules) == 0
and rule_name not in self.config.disabled_rules
):
rule_config = self.config.rules_config.get(rule_name, RuleConfig())
self._rules[rule_name] = rule(rule_config=rule_config)

Expand Down
18 changes: 18 additions & 0 deletions tests/test_cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,24 @@ def test_invalid_options():
assert result.exit_code == 2 # pylint: disable=PLR2004


def test_selected_and_disabled_rule_options_mutually_exclusive():
"""Test selected and disabled rule options are mutually exclusive."""
runner = CliRunner()
with patch("dbt_score.cli.Config._load_toml_file"):
result = runner.invoke(
lint,
[
"--manifest",
"fake_manifest.json",
"--selected-rule",
"foo",
"--disabled-rule",
"bar",
],
)
assert result.exit_code == 2 # pylint: disable=PLR2004


def test_lint_existing_manifest(manifest_path):
"""Test lint with an existing manifest."""
runner = CliRunner()
Expand Down
27 changes: 27 additions & 0 deletions tests/test_rule_registry.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,33 @@ def test_disabled_rule_registry_discovery():
]


def test_selected_rule_registry_discovery():
"""Ensure only selected rules are discovered."""
config = Config()
config.selected_rules = ["tests.rules.nested.example.rule_test_nested_example"]
r = RuleRegistry(config)
r._load("tests.rules")
assert sorted(r._rules.keys()) == [
"tests.rules.nested.example.rule_test_nested_example"
]


def test_selected_and_disabled_rule_registry_discovery():
"""Ensure selected rules are run even if otherwise disabled.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice I see it does work as expected, still I think my comment makes sense; shouldn't we warn the user about this invalid config? It will be less magical to the users I think


This is prevented by the CLI, but want to confirm selected rules
are run even if disabled via pyproject.toml.
"""
config = Config()
config.selected_rules = ["tests.rules.nested.example.rule_test_nested_example"]
config.disabled_rules = ["tests.rules.nested.example.rule_test_nested_example"]
r = RuleRegistry(config)
r._load("tests.rules")
assert sorted(r._rules.keys()) == [
"tests.rules.nested.example.rule_test_nested_example"
]


def test_configured_rule_registry_discovery(valid_config_path):
"""Ensure rules are discovered and configured correctly."""
config = Config()
Expand Down