-
Notifications
You must be signed in to change notification settings - Fork 186
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
fix: unravel Optional to inner generic arg from instance #2172
base: devel
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for dlt-hub-docs canceled.
|
This could also potentially be moved up the call stack into dlt/dlt/extract/incremental/__init__.py Line 714 in 5a1cb69
|
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.
thanks for the fix and writing the test! pls see the review comments
dlt/common/typing.py
Outdated
@@ -438,6 +438,10 @@ def get_generic_type_argument_from_instance( | |||
""" | |||
orig_param_type = Any | |||
if cls_ := getattr(instance, "__orig_class__", None): | |||
# unfurl Optional[Incremental[...]] to Incremental[...] | |||
if is_optional_type(cls_): |
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.
you could also look at extract_inner_type
in common/typing.py
. it will unfurl other wrappers (ie. Annotated) and also nested cases
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! Got rid of the conditional check and just used extract_inner_type
instead.
tests/common/test_typing.py
Outdated
|
||
def test_get_generic_type_argument_from_instance() -> None: | ||
# generic contains hint | ||
instance = SimpleNamespace(__orig_class__=Incremental[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.
we are trying to keep the common module as the upstream dependency of all other top level modules. so it is better to not import from extract
here. You could just use any standard generic type for testing
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.
Great point - updated in a4e5c11
Addresses resources created by the
rest_api_resources
factory function exiting prematurely when trying to _join_external_scheduler and not inferring start/end dates from Airflow.Related Issues
Closes #2171
Additional Context