-
Notifications
You must be signed in to change notification settings - Fork 15
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
Support for dbt core changes to support mashumaro 3.15 #228
base: main
Are you sure you want to change the base?
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. |
dbt_common/contracts/config/base.py
Outdated
Given a dict of keys, update the current config from them, validate | ||
it, and return a new config with the updated values | ||
""" | ||
# dct = self.to_dict(omit_none=False) |
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.
nit: this probably should be cleaned up
dbt_common/contracts/config/base.py
Outdated
@@ -115,6 +115,28 @@ def same_contents(cls, unrendered: Dict[str, Any], other: Dict[str, Any]) -> boo | |||
"object": ["snapshot_meta_column_names"], | |||
} | |||
|
|||
@classmethod | |||
def update_from( | |||
cls, dct: Dict[str, Any], data: Dict[str, Any], adapter_config_cls: Type[BaseConfig] |
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.
naming nit: is there naming here that would better signify the difference between dct
and data
?
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.
Do you have any suggestions? orig_dict, new_dict?
@@ -150,30 +172,6 @@ def _merge_dicts(cls, src: Dict[str, Any], data: Dict[str, Any]) -> Dict[str, An | |||
) | |||
return result | |||
|
|||
def update_from( |
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 don't appear to be explicit usages of update_from
in dbt-adapters: https://github.com/search?q=repo%3Adbt-labs%2Fdbt-adapters%20update_from&type=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.
The signature change here looks like it will have a very minor impact (just to dbt-core). We could release a dbt-common major version bump for this change since it is technically breaking, but that might be overkill given its low usage. At least a minor bump makes sense to me though.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #228 +/- ##
==========================================
- Coverage 68.89% 68.69% -0.20%
==========================================
Files 52 52
Lines 3433 3434 +1
==========================================
- Hits 2365 2359 -6
- Misses 1068 1075 +7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
68aab35
to
c3f3284
Compare
Supports dbt-labs/dbt-core#11051
Description
Changes to validate config dicts prior to instantiating config objects with "from_dict".
Checklist
changie new
to create a changelog entry