-
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
[1/n][dagster-dlt] Move asset spec creation to DagsterDltTranslator.get_asset_spec #27207
Conversation
241afbe
to
df13790
Compare
df13790
to
56c5f7f
Compare
56c5f7f
to
fc08bd8
Compare
@@ -29,13 +61,13 @@ def get_asset_key(self, resource: DltResource) -> AssetKey: | |||
def get_auto_materialize_policy(self, resource: DltResource) -> Optional[AutoMaterializePolicy]: | |||
"""Defines resource specific auto materialize policy. | |||
|
|||
This method can be overriden to provide custom auto materialize policy for a dlt resource. | |||
This method can be overrdidden to provide custom auto materialize policy for a dlt resource. |
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.
There is a typo in the docstring - overrdidden
should be overridden
Spotted by Graphite Reviewer
Is this helpful? React 👍 or 👎 to let us know.
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.
Fixed in 538c4b3
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.
nice
Summary & Motivation
Move the asset spec creation from the asset decorator to a
get_asset_spec
method in the translator.Also updates adds tests. No additional test was added for group names, owners, automation condition and auto-materialize policies. It is not recommended to update these values by overriding
get_asset_spec
because they are not related to dlt specifically.How I Tested These Changes
Updated and new tests with BK