Begin using Mergeable supplied by dbt-common
#9509
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #9509 +/- ##
=======================================
Coverage 87.97% 87.97%
=======================================
Files 167 167
Lines 22150 22162 +12
=======================================
+ Hits 19486 19498 +12
Misses 2664 2664
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
We're currently in the process of moving the "data resource" portion on nodes to `dbt/artifacts`. Some of those artifacts depend on `Mergeable` which has been defined on core. In order to move the data resources to `dbt/artifacts`, we thus need to move `Mergeable` upstream of core. We moved `Mergeable` to [dbt-common](https://github.com/dbt-labs/dbt-common) in dbt-labs/dbt-common#59, and released this change in [dbt-common 0.1.3](https://pypi.org/project/dbt-common/0.1.3/). As such as, in order to unblock some of the `dbt/artifacts` migration work, we first need to update references to `Mergeable` in core to use the `dbt-common` definition. NOTE: We include changing over to `Replaceable` from `dbt-common` in this commit. This is because there wasn't a clean way to do it. If I moved the imports of `Replaceable` on in the files where we updated `Mergeable` then we would have left `Replaceable` in an inbetween state. If we had moved all instances of `Replaceable`, it'd be out of scope for the change. As such, it makes more sense to do that as a separate changeset.
QMalcolm
force-pushed
the
qmalcolm--9505-get-mergeable-from-common
branch
from
4133209
to
a496438
Compare
MichelleArk
approved these changes
Feb 2, 2024
|
Can we safely delete Mergeable from contracts/util.py at this point as well? |
Although we've removed the definition of `Mergeable` we've ensured the import paths are still available. We do this because this is under `contracts`, and the sudden disappearance from the import path might cause issues for community members using dbt-core as a library. Ideally we'd define a `Mergeable` class here that inherits the `dbt-common` definition and raise a deprecation warning on instantiation. However, we don't have an established strategy to do so.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
resolves #9505
Problem
We need to begin using
Mergeablefromdbt-commonso that we move some objects which depend on it to dbt/artifactsSolution
Replace
Mergeableuses withMergeablefrom dbt-commonChecklist