-
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
Define Macro and Group resources in dbt/artifacts #9439
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. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #9439 +/- ##
==========================================
- Coverage 87.85% 87.85% -0.01%
==========================================
Files 160 164 +4
Lines 21935 21965 +30
==========================================
+ Hits 19272 19298 +26
- Misses 2663 2667 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
macros: List[str] = field(default_factory=list) | ||
|
||
# 'in' on lists is O(n) so this is O(n^2) for # of macros | ||
def add_macro(self, value: str): |
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.
Went back and forth on whether this method belongs in dbt/artifacts given it is mutating / not read-only. An alternative would be to define the method more directly on the internal Macro
node class.
Decided to leave it as-is for now because DependsOn
inherits from MacroDependsOn
. We should revisit a design once we move DependsOn
though.
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.
For those looking in @MichelleArk and I have talked about this a bit in DMs. In my PR #9460 I plan on moving DependsOn
in full over to artifacts. We plan in the future to revist the design of both MacroDependsOn
and DependsOn
to perhaps simplify and improve them.
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 to me!
resolves #9382
resolves #9381
Problem
Solution
Macro
into resource (defined in dbt/artifcats/resources/v1) and internal representationDocs
to dbt/artifactsDocumentationArtifact
toDocumentationResource
for consistent namingGroup
into functional & resource classesBaseArtifactNode
toBaseResource
Checklist