-
Notifications
You must be signed in to change notification settings - Fork 182
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 add_limit wrapper for async generators #1310
Conversation
✅ Deploy Preview for dlt-hub-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
if i is not None: | ||
count += 1 | ||
# async gen yields awaitable so we must count one awaitable more | ||
# so the previous one is evaluated and yielded. | ||
# new awaitable will be cancelled | ||
if count == max_items + int(is_async_gen): | ||
return | ||
yield i |
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 need to yield only after if
section
769874a
to
33b5af6
Compare
while True: | ||
if should_stop: | ||
break | ||
yield loop.run_until_complete(gen.__anext__()) # type: ignore[arg-type] |
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.
I used eventloop because with the old implementation it has been complaining about not awaited generator when resource combined with .add_limit
tests/extract/test_sources.py::test_add_limit_async
/[...]/dlt/dlt/extract/pipe_iterator.py:275: RuntimeWarning: coroutine 'wrap_async_iterator.<locals>.run' was never awaited
pipe_item = next(gen)
Enable tracemalloc to get traceback where the object was allocated.
See https://docs.pytest.org/en/stable/how-to/capture-warnings.html#resource-warnings for more info.
33b5af6
to
8e7fcd1
Compare
This PR is an attempt to address #1213