-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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.
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
CT 1537 fix event test and rename a couple of fields #6293
Conversation
Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more rename and one question.
@@ -16,7 +16,7 @@ | |||
disallow_secret_env_var, | |||
) | |||
from dbt.events.functions import fire_event, get_invocation_id | |||
from dbt.events.types import MacroEventInfo, MacroEventDebug | |||
from dbt.events.types import JinjaLogInfo, JinjaLogDebug |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Along these same lines there's also a GeneralMacroWarning
that is the catchall for warn
defined in exceptions.py
. It's worth renaming that along with these.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't look like it's actually used anywhere. Can we just remove it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's used within macros (docs)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So it's exported into the context. So should we call it JinjaLogWarning?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or JinjaLogWarn?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went with JinjaLogWarning.
@@ -15,6 +15,7 @@ message EventInfo { | |||
string thread = 7; | |||
google.protobuf.Timestamp ts = 8; | |||
map<string, string> extra = 9; | |||
string category = 10; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this eventually going to be the log category? Am I missing how this gets populated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, right now it's just a placeholder. Eventually we'll populate it somehow...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there an issue so we don't lose the fact we still need to actually populate it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly it's your ticket #5958.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
resolves #6292
Description
The event unit test was broken and some of the tests weren't actually doing anything. This fixes that, removes the lists of events from the bottom of the types.py and test_types.py files and moves them into test_events.py test file. It also renames a few tests.
Checklist
changie new
to create a changelog entry