Only output failing models and violated rules per default in HumanReadableFormatter #77
Conversation
| score.value < self._config.fail_any_model_under | ||
| or any(isinstance(result, RuleViolation) for result in results.values()) | ||
| or self._config.show_all |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
- show failing models, with all rules
- show failing models, with failing rules
- show all models, with all rules
- show all models, with failing rules
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
Sounds good and happy to adjust it in the next few days - thanks for all the input.
There was a problem hiding this comment.
In terms of wording: The codebase often refers to "violated" rules, e.g. RuleViolation and not failing rules. What do you think of:
--show all
--show failing-models
--show violated-rules
?
There was a problem hiding this comment.
I think that makes sense! 👍
There was a problem hiding this comment.
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
| @@ -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" | |||
There was a problem hiding this comment.
Why did all the B's turn into b? 😁
There was a problem hiding this comment.
Hexadecimal is not case sensitive, but indeed strange to see those changed 🤔
There was a problem hiding this comment.
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.
Co-authored-by: Jochem van Dooren <[email protected]>
Co-authored-by: Jochem van Dooren <[email protected]>
| @click.option( | ||
| "--show-all", | ||
| 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.", | ||
| type=bool, | ||
| is_flag=True, | ||
| default=False, | ||
| ) |
There was a problem hiding this comment.
Let's not make it a flag, but rather a parameter with a value? i.e. --show all, which can later be expanded into different options such as --show failing-models and the like
Fixes #71
It changes the default behavior of the HumanReadableFormatter:
When running
dbt-score lintit will now only show failing models and/or rules which were violated.To have all output printed (exactly the way it was before this PR) the user has now to pass the
--show_allflag todbt-score lint.It also considers the
fail_any_model_underoption.Furthermore, I had to adjust some of tests since the default behavior of the HumanReadableFormatter is now changed - let me know if I would need to expand this.
The docs also reflect this change by slightly tweaking the first example in
index.mdIn this current form, both rule violations and failing models are filtered through the same argument. This is based on @matthieucan. 's comment on #71 (comment)