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.

Sign up for GitHub

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

Only output failing models and violated rules per default in HumanReadableFormatter #77

Merged
merged 19 commits into from
Dec 18, 2024
Merged

Only output failing models and violated rules per default in HumanReadableFormatter #77

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
ce39962
only_show_failing argument
thomend Sep 29, 2024
16f8079
added flag and changed default behavior to only show rules/models wh…
thomend Oct 1, 2024
321a846
changed HR formater to print Violations of all failing rules
thomend Oct 2, 2024
1cd404d
removed redudant print statement
thomend Oct 2, 2024
7fd5b74
changed doc example to showcase new default behavior
thomend Oct 2, 2024
94a1c69
renamed CLI argument from show_all to show-all
thomend Oct 4, 2024
b752a41
Update docs/index.md
thomend Oct 7, 2024
48abfc1
Update src/dbt_score/cli.py
thomend Oct 7, 2024
512b04f
changed --show from flag to option
thomend Dec 3, 2024
f2b02a1
resolved merge conflicts
thomend Dec 3, 2024
0f8bfd8
replaced instancers of model with item
thomend Dec 11, 2024
0b1dc0b
improved docstring
thomend Dec 11, 2024
3928d56
fixed if condition for printing output from evaluable
thomend Dec 11, 2024
667908f
fixed output for --failing-items mode
thomend Dec 11, 2024
583cc6b
Update docs/index.md
thomend Dec 11, 2024
ff241de
added tests for show parameter
thomend Dec 12, 2024
3c9df1d
added entry in changelog
thomend Dec 12, 2024
ce49628
Update CHANGELOG.md
thomend Dec 17, 2024
825c48b
run prettier to fix issue in docs
thomend Dec 17, 2024
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
5 changes: 3 additions & 2 deletions docs/index.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ encourage) good practices.
## Example

```
> dbt-score lint
> dbt-score lint --show_all
🥇 customers (score: 10.0)
OK dbt_score.rules.generic.has_description
OK dbt_score.rules.generic.has_owner: Model lacks an owner.
Expand All @@ -21,7 +21,8 @@ Score: 10.0 🥇

In this example, the model `customers` scores the maximum value of `10.0` as it
passes all the rules. It also is awarded a golden medal because of the perfect
score.
score. By default a passing model without rule violations will only be shown if
we pass the `---show_all` flag.
thomend marked this conversation as resolved.
Show resolved Hide resolved

## Philosophy

Expand Down
15 changes: 13 additions & 2 deletions src/dbt_score/cli.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,8 +93,17 @@ def cli() -> None:
is_flag=False,
default=None,
)
@click.option(
"--show_all",
thomend marked this conversation as resolved.
Show resolved Hide resolved
help="If set to True,show all models and all rules in output"
"when using `plain` as `--format`."
"Default behavior is to only show failing models and violated rules",
thomend marked this conversation as resolved.
Show resolved Hide resolved
type=bool,
is_flag=True,
default=False,
)
@click.pass_context
def lint(
def lint( # noqa: PLR0913, C901
ctx: click.Context,
format: Literal["plain", "manifest", "ascii"],
select: tuple[str],
Expand All @@ -104,6 +113,7 @@ def lint(
run_dbt_parse: bool,
fail_project_under: float,
fail_any_model_under: float,
show_all: bool,
) -> None:
"""Lint dbt models metadata."""
manifest_provided = (
Expand All @@ -123,7 +133,8 @@ def lint(
config.overload({"fail_project_under": fail_project_under})
if fail_any_model_under:
config.overload({"fail_any_model_under": fail_any_model_under})

if show_all:
config.overload({"show_all": show_all})
try:
if run_dbt_parse:
dbt_parse()
Expand Down
2 changes: 2 additions & 0 deletions src/dbt_score/config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ class Config:
"inject_cwd_in_python_path",
"fail_project_under",
"fail_any_model_under",
"show_all",
]
_rules_section: Final[str] = "rules"
_badges_section: Final[str] = "badges"
Expand All @@ -71,6 +72,7 @@ def __init__(self) -> None:
self.badge_config: BadgeConfig = BadgeConfig()
self.fail_project_under: float = 5.0
self.fail_any_model_under: float = 5.0
self.show_all: bool = False

def set_option(self, option: str, value: Any) -> None:
"""Set an option in the config."""
Expand Down
36 changes: 24 additions & 12 deletions src/dbt_score/formatters/human_readable_formatter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,18 +33,30 @@ def model_evaluated(
"""Callback when a model has been evaluated."""
if score.value < self._config.fail_any_model_under:
self._failed_models.append((model, score))
print(f"{score.badge} {self.bold(model.name)} (score: {score.rounded_value!s})")
for rule, result in results.items():
if result is None:
print(f"{self.indent}{self.label_ok} {rule.source()}")
elif isinstance(result, RuleViolation):
print(
f"{self.indent}{self.label_warning} "
f"({rule.severity.name.lower()}) {rule.source()}: {result.message}"
)
else:
print(f"{self.indent}{self.label_error} {rule.source()}: {result!s}")
print()
if (
score.value < self._config.fail_any_model_under
or any(isinstance(result, RuleViolation) for result in results.values())
or self._config.show_all
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is not 100% correct 🤔 If I understand correctly we want to print the following output if:

score.value < self._config.fail_any_model_under. Then, only show the failing rules. Now it will also show the failing rules of models that did not fail

Copy link
Contributor Author

@thomend thomend Oct 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

That's a good point! My line of thought here was, that in case that a project as whole fails and only very few model scores are too low , I probably would be interested as a user to also see failing rules of all models: Imagine you have 100 models, only ~5 fail but ~20 have lowish scores while 75 are perfect. In that case it could be of interest to also see the failing rules of all models.

I also referred to this in the issue discussion: #71 (comment). But I guess in that case one could just increase fail_any_model_under. Probably this is a bit too implicit and I could remove this (e.g only test for score.value < self._config.fail_any_model_under).

Just let me know which way you prefer and then I adjust it accordingly.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok I got it! 👍 I think for the scenario you describe it is indeed useful to be able to show all the failing rules and we can definitely leave that as the default. I do think that we should also have the option to show only failing models, with their failing rules.

Maybe we should have two flags: --show-all-rules and --show-all-models so the user is able to further specify the output. @matthieucan curious to hear your opinion as well!

So then the user is able to:

  1. show failing models, with all rules
  2. show failing models, with failing rules
  3. show all models, with all rules
  4. show all models, with failing rules

Copy link
Contributor

Choose a reason for hiding this comment

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

Good to align on those expectations indeed.
Maybe the --show parameter could take an argument, e.g.

  • --show all - show all models, all rules
  • --show failing-models - show failing rules of failing models
  • --show failing-rules - show failing rules of all models (the default?)

I'm not sure if the first scenario mentioned (show failing models, with all rules) is useful, considering the option to use --select in combination. For example --select my_model --show all might be more actionable. But let me know what you think :)

Copy link
Contributor

Choose a reason for hiding this comment

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

You are right that option 1 (failing models with all rules) is probably not very useful. I think the direction of --show something would be a nice one. It's indeed simpler than providing two flags. And agreed that --show failing-rules should be the default!

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's simpler to remember for users if the same term is used for the CLI options, as it's an abstraction over the code base

Copy link
Contributor

@jochemvandooren jochemvandooren Nov 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Hey @thomend! Will you be able to continue on this PR sooner or later? We can assist if needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @jochemvandooren sorry not picking it up earlier! I initially planned to work on it much sooner again. I am picking it up today evening again and will come back to you asap then - hope that works for you.

Copy link
Contributor Author

@thomend thomend Dec 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I just added the different options as discussed above. I also took care of the merge conflicts. Tests run through + precommit hooks as well.
The show parameter takes the following options now:

--show all
--show failing-models
--show failig-rules (default)

Copy link
Contributor

Choose a reason for hiding this comment

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

No problem @thomend ! Was afraid you had forgotten 😢 I will review tomorrow!

):
print(
f"{score.badge} {self.bold(model.name)} "
f"(score: {score.rounded_value!s})"
)
for rule, result in results.items():
if result is None:
if self._config.show_all:
print(f"{self.indent}{self.label_ok} {rule.source()}")
elif isinstance(result, RuleViolation):
print(
f"{self.indent}{self.label_warning} "
f"({rule.severity.name.lower()}) "
f"{rule.source()}: {result.message}"
)
else:
print(
f"{self.indent}{self.label_error} {rule.source()}: {result!s}"
)
print()

def project_evaluated(self, score: Score) -> None:
"""Callback when a project has been evaluated."""
Expand Down
74 changes: 56 additions & 18 deletions tests/formatters/test_human_readable_formatter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,41 @@ def test_human_readable_formatter_model(
stdout = capsys.readouterr().out
assert (
stdout
== """🥇 \x1B[1mmodel1\x1B[0m (score: 10.0)
\x1B[1;32mOK \x1B[0m tests.conftest.rule_severity_low
\x1B[1;31mERR \x1B[0m tests.conftest.rule_severity_medium: Oh noes
\x1B[1;33mWARN\x1B[0m (critical) tests.conftest.rule_severity_critical: Error
== """🥇 \x1b[1mmodel1\x1b[0m (score: 10.0)
\x1b[1;31mERR \x1b[0m tests.conftest.rule_severity_medium: Oh noes
\x1b[1;33mWARN\x1b[0m (critical) tests.conftest.rule_severity_critical: Error

"""
)


def test_human_readable_formatter_model_show_all(
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we have a test for failing-models as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a parametrized test to test for the different options in the show parameter - hope this works :)

capsys,
default_config,
manifest_loader,
model1,
rule_severity_low,
rule_severity_medium,
rule_severity_critical,
):
"""Ensure the formatter has the correct output after model evaluation."""
default_config.overload({"show_all": True})
formatter = HumanReadableFormatter(
manifest_loader=manifest_loader, config=default_config
)
results: ModelResultsType = {
rule_severity_low: None,
rule_severity_medium: Exception("Oh noes"),
rule_severity_critical: RuleViolation("Error"),
}
formatter.model_evaluated(model1, results, Score(10.0, "🥇"))
stdout = capsys.readouterr().out
assert (
stdout
== """🥇 \x1b[1mmodel1\x1b[0m (score: 10.0)
\x1b[1;32mOK \x1b[0m tests.conftest.rule_severity_low
\x1b[1;31mERR \x1b[0m tests.conftest.rule_severity_medium: Oh noes
\x1b[1;33mWARN\x1b[0m (critical) tests.conftest.rule_severity_critical: Error

"""
)
Expand All @@ -44,7 +75,7 @@ def test_human_readable_formatter_project(capsys, default_config, manifest_loade
)
formatter.project_evaluated(Score(10.0, "🥇"))
stdout = capsys.readouterr().out
assert stdout == "Project score: \x1B[1m10.0\x1B[0m 🥇\n"
assert stdout == "Project score: \x1b[1m10.0\x1b[0m 🥇\n"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did all the B's turn into b? 😁

Copy link
Contributor

Choose a reason for hiding this comment

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

Hexadecimal is not case sensitive, but indeed strange to see those changed 🤔

Copy link
Contributor Author

@thomend thomend Oct 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry for that! I am using the ruff vscode extension and the autoformat on save did that:
Hex codes and Unicode sequences. Although I don't know why the linter during pre-commit would not pick it up to revert it? The ruff version of the vscode extension is 0.6.6, I believe (which is newer than the one running in the pre-commit hook.
Let me know if you want it reverted.



def test_human_readable_formatter_near_perfect_model_score(
Expand All @@ -57,6 +88,7 @@ def test_human_readable_formatter_near_perfect_model_score(
rule_severity_critical,
):
"""Ensure the formatter has the correct output after model evaluation."""
default_config.overload({"show_all": True})
formatter = HumanReadableFormatter(
manifest_loader=manifest_loader, config=default_config
)
Expand All @@ -69,10 +101,10 @@ def test_human_readable_formatter_near_perfect_model_score(
stdout = capsys.readouterr().out
assert (
stdout
== """🥈 \x1B[1mmodel1\x1B[0m (score: 9.9)
\x1B[1;32mOK \x1B[0m tests.conftest.rule_severity_low
\x1B[1;31mERR \x1B[0m tests.conftest.rule_severity_medium: Oh noes
\x1B[1;33mWARN\x1B[0m (critical) tests.conftest.rule_severity_critical: Error
== """🥈 \x1b[1mmodel1\x1b[0m (score: 9.9)
\x1b[1;32mOK \x1b[0m tests.conftest.rule_severity_low
\x1b[1;31mERR \x1b[0m tests.conftest.rule_severity_medium: Oh noes
\x1b[1;33mWARN\x1b[0m (critical) tests.conftest.rule_severity_critical: Error

"""
)
Expand All @@ -87,7 +119,7 @@ def test_human_readable_formatter_near_perfect_project_score(
)
formatter.project_evaluated(Score(9.99, "🥈"))
stdout = capsys.readouterr().out
assert stdout == "Project score: \x1B[1m9.9\x1B[0m 🥈\n"
assert stdout == "Project score: \x1b[1m9.9\x1b[0m 🥈\n"


def test_human_readable_formatter_low_model_score(
Expand All @@ -107,28 +139,34 @@ def test_human_readable_formatter_low_model_score(
formatter.model_evaluated(model1, results, Score(0.0, "🚧"))
formatter.project_evaluated(Score(0.0, "🚧"))
stdout = capsys.readouterr().out
print(stdout)
print()
assert (
stdout
== """🚧 \x1B[1mmodel1\x1B[0m (score: 0.0)
\x1B[1;33mWARN\x1B[0m (critical) tests.conftest.rule_severity_critical: Error
== """🚧 \x1b[1mmodel1\x1b[0m (score: 0.0)
\x1b[1;33mWARN\x1b[0m (critical) tests.conftest.rule_severity_critical: Error

Project score: \x1B[1m0.0\x1B[0m 🚧
Project score: \x1b[1m0.0\x1b[0m 🚧

Error: model score too low, fail_any_model_under = 5.0
Model model1 scored 0.0
"""
)


def test_human_readable_formatter_low_project_score(
def test_human_readable_formatter_low_project_score_high_model_score(
capsys,
default_config,
manifest_loader,
model1,
rule_severity_critical,
):
"""Ensure the formatter has the correct output when the projet has a low score."""
"""Ensure the formatter has the correct output when the projet has a low score.

If model itself has a high project score then we need to pass `show_all` flag
to make it visible.
"""
default_config.overload({"show_all": True})
formatter = HumanReadableFormatter(
manifest_loader=manifest_loader, config=default_config
)
Expand All @@ -141,10 +179,10 @@ def test_human_readable_formatter_low_project_score(
print()
assert (
stdout
== """🥇 \x1B[1mmodel1\x1B[0m (score: 10.0)
\x1B[1;33mWARN\x1B[0m (critical) tests.conftest.rule_severity_critical: Error
== """🥇 \x1b[1mmodel1\x1b[0m (score: 10.0)
\x1b[1;33mWARN\x1b[0m (critical) tests.conftest.rule_severity_critical: Error

Project score: \x1B[1m0.0\x1B[0m 🚧
Project score: \x1b[1m0.0\x1b[0m 🚧

Error: project score too low, fail_project_under = 5.0
"""
Expand Down
3 changes: 1 addition & 2 deletions tests/test_cli.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ def test_lint_existing_manifest(manifest_path):
"""Test lint with an existing manifest."""
with patch("dbt_score.cli.Config._load_toml_file"):
runner = CliRunner()
result = runner.invoke(lint, ["--manifest", manifest_path])
result = runner.invoke(lint, ["--manifest", manifest_path, "--show_all"])

assert "model1" in result.output
assert "model2" in result.output
Expand Down Expand Up @@ -93,7 +93,6 @@ def test_fail_any_model_under(manifest_path):
result = runner.invoke(
lint, ["--manifest", manifest_path, "--fail_any_model_under", "10.0"]
)

assert "model1" in result.output
assert "model2" in result.output
assert "Error: model score too low, fail_any_model_under" in result.stdout
Expand Down