Event logger config takes instance instead of class #7997
Conversation
|
Side note: Try running |
betodealmeida
left a comment
There was a problem hiding this comment.
Nice! I have just a comment on using textwrap.dedent, and a few nits.
superset/utils/log.py
Outdated
| """ | ||
| result: Any = cfg_value | ||
| if inspect.isclass(cfg_value): | ||
| logging.getLogger().warning( |
There was a problem hiding this comment.
We could also use https://docs.python.org/3/library/exceptions.html#DeprecationWarning here. TBH I'm not super familiar with the difference or the best approach.
There was a problem hiding this comment.
I feel like this is probably a more "correct" approach, but I tried to use warnings.warn(msg, DeprecationWarning) and didn't see any message locally. I'm sure it can be configured, but I wanted to be sure that this would be seen.
There was a problem hiding this comment.
Other than this one (which I am open to, but it seems... extra) I implemented all of your suggestions @betodealmeida
|
@dpgaspar one more ping |
CATEGORY
Choose one
SUMMARY
Deprecate EVENT_LOGGER assignment of class type in favor of instance. Deprecation is handled gracefully and only outputs a WARNING log message at config load time. We have a need to configure an impl of AbstractEventLogger which is instantiated/pre-configured with some proprietary dependencies.
This change is a small refactor on top of the nice change #7705 @dpgaspar made a few weeks ago.
TEST PLAN
Unit tests added, and tested manually.
ADDITIONAL INFORMATION
REVIEWERS
@dpgaspar @john-bodley @graceguo-supercat @mistercrunch