CT 1644 node cleanup #6427
CT 1644 node cleanup #6427
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. |
gshank
requested review from
peterallenwebb
and removed request for
iknox-fa and
McKnight-42
gshank
force-pushed
the
ct-1644-node_cleanup
branch
from
4580704 to
a27f2d2
Compare
| if not isinstance(node, (ParsedNode)): | ||
| raise InternalException(f"type mismatch, expected ParsedNode but got {type(node)}") | ||
| raise InternalException( | ||
| f"type mismatch, expected ManifestNode but got {type(node)}" |
There was a problem hiding this comment.
This error and the check don't match.
There was a problem hiding this comment.
They're actually the same thing. Mypy wouldn't let me use ManifestNode in an isinstance check. I'd rather not use ParsedNode in the message, because I think using the superclasses is more confusing...
There was a problem hiding this comment.
Maybe just a comment in the code to that effect then?
There was a problem hiding this comment.
In any case, it's really not a user-facing exception. It should only show up when a developer has made a mistake :)
There was a problem hiding this comment.
Sure, I just want to avoid spending 5 minutes wondering why these don't match next time I'm in there 😄
There was a problem hiding this comment.
The comment "# Can't use ManifestNode here because of parameterized generics" wasn't enough?
| # ===================================================================== | ||
| # This contains the classes for all of the nodes and node-like objects | ||
| # in the manifest. In the "nodes" dictionary of the manifest we find | ||
| # all of the objects in the ManifestNode union below. In addition the | ||
| # manifest contains "macros", "sources", "metrics", "exposures", "docs", | ||
| # and "disabled" dictionaries. | ||
| # | ||
| # The SeedNode is a ManifestNode, but can't be compiled because it has | ||
| # no SQL. | ||
| # | ||
| # All objects defined in this file should have BaseNode as a parent | ||
| # class. | ||
| # | ||
| # The two objects which do not show up in the DAG are Macro and | ||
| # Documentation. | ||
| # ===================================================================== |
resolves #6426
Description
This pull request contains a lot of cleanup of the manifest nodes.
Note: core/dbt/contracts/graph/nodes.py has changed so much that I don't recommend trying to read the diff. Reviewers should read the changed file instead.
Checklist
changie newto create a changelog entry