Split Parsed and Compiled nodes into subtypes (#1601) #1610
Conversation
beckjake
force-pushed
the
feature/dataclasses-jsonschema-types
branch
from
22a480b
to
49f7cf8
Compare
beckjake
force-pushed
the
feature/split-parsed-things
branch
from
5f7d804
to
e4f37ab
Compare
A few test changes to account for removed fields based on types
beckjake
force-pushed
the
feature/split-parsed-things
branch
from
e4f37ab
to
12e53c7
Compare
beckjake
changed the base branch from
feature/dataclasses-jsonschema-types
to
dev/louisa-may-alcott
|
|
||
| def parse_from_dict(self, parsed_dict: Dict[str, Any]) -> ParsedRPCNode: | ||
| """Given a dictionary, return the parsed entity for this parser""" | ||
| return ParsedRPCNode.from_dict(parsed_dict) |
There was a problem hiding this comment.
all of these duplicated parse_from_dicts actually feel like an unfortunate side effect of using type annotations. is there a way to use a "generic" here and specify which node type each Parser generates?
this case actually doesn't feel like the biggest deal. this code is simple. but i can see this being troublesome in the future when we want to inherit functionality and have to redefine the entire method to annotate the return type
There was a problem hiding this comment.
Yeah. The real thing I want is for MacrosKnownParser (or something?) to subclass Generic[T] where T is something ParsedNode-like and then subclasses would be like:
class AnalysisParser(MacrosKnownParser[ParsedAnalysisNode]):
...
I just didn't really want to spend a lot of time messing with parsing class definitions in this PR only to rip them out or be deeply inconvenienced in the PRs for #1487 or #1600 where I imagine I'll have to make a ton of changes. typing.Generic can result in some seriously weird errors because it does some metaclass craziness.
beckjake
force-pushed
the
feature/split-parsed-things
branch
from
c00f9d0
to
b9a3fe5
Compare
Fixes #1601
Fixes #1486
This includes a few test changes to account for removed fields based on types
This PR doesn't remove the
raw_sqlfield from seeds or anything like that, as that's a bit more involved - right now dbt really assumes that all nodes have that field and it's easier to leave it for the moment. In the future when compilation is cleaned up a bit it will probably make sense to remove that.