Skip to content
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

355 - add computed exhausted property #380

Merged
merged 6 commits into from
Jun 13, 2023

Conversation

sh-rp
Copy link
Collaborator

@sh-rp sh-rp commented Jun 5, 2023

No description provided.

@netlify
Copy link

netlify bot commented Jun 5, 2023

Deploy Preview for dlt-hub-docs canceled.

Name Link
🔨 Latest commit a9be6cd
🔍 Latest deploy log https://app.netlify.com/sites/dlt-hub-docs/deploys/6488ad5401e13600087d64df

Copy link
Collaborator

@rudolfix rudolfix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please link to issue in PR. maybe we should a template?

@@ -614,6 +614,15 @@ def root_key(self) -> bool:
config = RelationalNormalizer.get_normalizer_config(self._schema).get("propagation")
return config is not None and "root" in config and "_dlt_id" in config["root"] and config["root"]["_dlt_id"] == "_dlt_root_id"

@property
def exhausted(self) -> bool:
# check all selected pipes wether one of them is exhausted
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

better to use docstrings

def exhausted(self) -> bool:
# check all selected pipes wether one of them is exhausted
for resource in self._resources.extracted.values():
for item in resource._pipe:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you do not need to enumerate items. Pipe has gen property which is the main generator of the pipe. all others are transforms. so check only gen

# check all selected pipes wether one of them is exhausted
for resource in self._resources.extracted.values():
for item in resource._pipe:
if isinstance(item, types.GeneratorType):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

interesting! I'm using inspect.isgenerator and it's probably better to stick to that (or you double check if the above catches all cases of generators)

Copy link
Collaborator

@rudolfix rudolfix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also the linter is not passing

def some_data():
yield from [1, 2, 3, 4]

s = DltSource("source", "module", Schema("source"), [dlt.resource(some_data())])
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all good, but please add more resource types to source

  • a regular list (iterable)
  • an iterator (ie. on a list)
  • join gen resource to transformer

I just want to make sure all cases are passing, you can do it on one test

@sh-rp sh-rp force-pushed the d#/355-proper-exhausted-property branch from 946bb52 to 788994f Compare June 11, 2023 16:39
@rudolfix rudolfix force-pushed the d#/355-proper-exhausted-property branch from c69968e to 6c7dd39 Compare June 12, 2023 14:12
Copy link
Collaborator

@rudolfix rudolfix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK! finally we have all the tests and CI is passing!

@rudolfix rudolfix merged commit 778f3f9 into devel Jun 13, 2023
31 checks passed
@rudolfix rudolfix deleted the d#/355-proper-exhausted-property branch June 13, 2023 20:43
@rudolfix rudolfix restored the d#/355-proper-exhausted-property branch June 13, 2023 20:44
@rudolfix rudolfix deleted the d#/355-proper-exhausted-property branch June 18, 2023 19:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants