Add ability to silence events via WarnErrorOptions
#112
Conversation
dbt_common/helper_types.py
Outdated
| @@ -36,6 +36,7 @@ class IncludeExclude(dbtClassMixin): | |||
|
|
|||
| include: Union[str, List[str]] | |||
| exclude: List[str] = field(default_factory=list) | |||
| silence: List[str] = field(default_factory=list) | |||
There was a problem hiding this comment.
I believe a more appropriate place for this attribute would be on WarnErrorOptions itself, since IncludeExclude can be used by other interfaces as well that do not support silence. The silence functionality feels specific to the WarnErrorOptions mechanism itself -- which could override includes (and still probably leverage super().includes) to incorporate the business logic of handling self.silenced.
There was a problem hiding this comment.
We can do that. We'd also need to override the __post_init__ to validate the silence options. I'm not a huuuge fan of inheriting a class and then having to override every part of it though. Given these classes are still small it isn't a big deal, but much further I'd start strongly considering disconnecting or collapsing the classes.
There was a problem hiding this comment.
I'm not a huuuge fan of inheriting a class and then having to override every part of it though.
Me neither, but given there is usage of IncludeExclude beyond the WarnErrorOptions class in dbt-core (e.g. in UnparsedVersion.columns), I'm nervous this will cause unintended changes to additional components beyond WarnErrorOptions.
There was a problem hiding this comment.
That makes sense. You're absolutely right, the silence option really only makes sense from a logging standpoint, which is the privy of WarnErrorOptions, not IncludeExclude. I've gone and done the refactor in 61f7ff9.
Worth noting that the risk pre-refactor was low. This is because anything using IncludeExclude would not have changed behaviorly unless they began specifying silence
- if nothing was silenced, everything operates normally
- if instantiating with keyword args, and not specifying
silence, thensilencewas empty - if instantiating with ordered args, and only specifying two args, then
silencewas empty
Technically since we modified `includes` in the previous commit, `warn_or_error` started including `silenced` in it's logic then. However we make it explicit in this commit, and begin testing it.
…es defined in tests In the previous commit we defined a `Note` event class based on `WarnLevel`. The logic in `get_all_subclasses` was getting test defined event classes, which isn't what we want. Thus here we added logic to exclude these.
QMalcolm
force-pushed
the
qmalcolm--111-warn-error-silence-option
branch
from
0785b51 to
688556f
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #112 +/- ##
==========================================
+ Coverage 55.79% 56.26% +0.46%
==========================================
Files 50 50
Lines 3072 3082 +10
==========================================
+ Hits 1714 1734 +20
+ Misses 1358 1348 -10
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| @@ -54,7 +54,7 @@ def __post_init__(self): | |||
| if isinstance(self.exclude, list): | |||
| self._validate_items(self.exclude) | |||
|
|
|||
| def includes(self, item_name: str): | |||
| def includes(self, item_name: str) -> bool: | |||
resolves #111
Description
We want to be able to silence events via
warn_or_error. This enables that.Checklist
changie newto create a changelog entry