-
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
convert contract to dict #7222
convert contract to dict #7222
Conversation
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 have a question about the use of a dictionary here. In my PR (also up for review) I'm adding a "contract_checksum" as a separate field, but which I assume will go in this new contract "thing". Do we actually want it to be a dictionary or an object with fields, such as "enabled" and "checksum"? There are pros and cons for each...
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, except for the location of the Contract class.
core/dbt/contracts/graph/unparsed.py
Outdated
@@ -87,6 +87,11 @@ class Docs(dbtClassMixin, Replaceable): | |||
node_color: Optional[str] = None | |||
|
|||
|
|||
@dataclass | |||
class Contract(dbtClassMixin, Replaceable): |
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 the Contract class is used by anything in unparsed.py, so it ought to go in nodes.py. We should limit what nodes.py uses from unparsed.py as much as possible.
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.
That creates circular imports with model_config. Instead of nodes.py
I moved it into model_config.py
. The only other thing defined in there is Hook
but I think it still makes sense since it's not used in unparsed.py
.
449aa48
to
3d96f5b
Compare
resolves #7184
Description
Converts contract from a bool to it's own object
contract: bool = False
to
Checklist
changie new
to create a changelog entry