-
Notifications
You must be signed in to change notification settings - Fork 4
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
Feature: Implement ChangeSet
file editing paradigm
#107
Feature: Implement ChangeSet
file editing paradigm
#107
Conversation
…tances of a resource not existing yet
…t. Correct defect in ChangeSet iterator
…ts to dicts, and back again
… file managers Why get rid of the DbtFileManager? It returned mutually exclusive types! When this happens, we're stuck doing type checks every time we read a file, which is a nightmare. Secondly, the DbtFileManager had assumptions about realtive paths being used in oeprations, which as of ChangeSets is NO LONGER THE CASE. Now, absolute paths are used everywhere. As a result, it made sense to split out our concerns quite a bit such that there is a RawFileManager that handles I/O of raw data, and a YamlFileManager that adds in a YAML parsing layer to keep our code clean.
Co-authored-by: dave-connors-3 <[email protected]>
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.
Oops! All to-dos
@nicholasyager: Feedback from the sync
|
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.
at first glance, nothing really jumping out to me except holy cow is this exceptionally readable. The comments here aren't really blocking at all, mostly questions, clarifications, and notes to self. I'm gonna keep using it a but more, but at the moment not seeing any reason not to approve
class BaseFileManager(ABC): | ||
@abc.abstractmethod | ||
def read_file(self, path: Path) -> Union[Dict[str, Any], str, None]: | ||
class FileManager(Protocol): |
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.
total curiosity question: what's a Protocol
? similar to an abstract base class?
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.
A Protocol is similar to an Abstract Base Class, but it uses duck typing instead of inheritance. So, for instance, we don't need to have a bunch of file managers inheriting from an abstract file manager. Instead, we can define a protocol, and any class that implements the correct methods will be allowed via the type checker. It's rather similar to the trait system in rust, which I LOVE.
} | ||
|
||
@staticmethod | ||
def update_refs__sql(model_name, project_name, model_code): |
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.
note to future selves: will need to handle optional version args in refs before too long!
If it ain't being used, then yoink 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.
I am comfortable merging these changes -- incredible work on this @nicholasyager , a wholesale rewrite is no small task! can't wait to show off --dry-run
!
Description and Motivations
This PR is a large refactoring effort that implements
ChangeSets
. Change sets are a standardized interface for describing resource and file changes within a dbt project. Whereas before we would have the DbtMeshConstructor and DbtProjectEditor classes to perform bespoke operations on a dbt project, now our operations yieldChanges
that describe the specific modifications to perform. This decoupling from identification and operation allows us to handle all operations at arbitrary times, like after we've logged our plan or even not at all (a dry run)!As a byproduct of this change, there were also a number of opportunities to simplify our codebase considerably. Now we no longer required the following:
Resolves: #16
Resolves: #67
Resolves: #69
Resolves: #108
Example Run