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

Add paginate shortcut to dlt.sources.helpers.requests #1212

Closed
wants to merge 1 commit into from

Conversation

burnash
Copy link
Collaborator

@burnash burnash commented Apr 11, 2024

Description

This PR adds a new function paginate to dlt.sources.helpers.requests avoiding the circular import issue from #1154
paginate is a shortcut which creates a RESTClient under the hood and runs paginate() method.

Related PRs

@burnash burnash requested a review from rudolfix April 11, 2024 12:58
Copy link

netlify bot commented Apr 11, 2024

Deploy Preview for dlt-hub-docs ready!

Name Link
🔨 Latest commit 4176cfb
🔍 Latest deploy log https://app.netlify.com/sites/dlt-hub-docs/deploys/6617de6b31a32d00088d7eb4
😎 Deploy Preview https://deploy-preview-1212--dlt-hub-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@burnash
Copy link
Collaborator Author

burnash commented Apr 11, 2024

@rudolfix, it works now; however, I'm not fully convinced that dlt.sources.helpers.requests is an appropriate place for the paginate() shortcut function. I believe it still logically belongs in the rest_client namespace. A real-world example is when someone needs to explicitly use an authenticator or a paginator; they would need to import that from the dlt.sources.helpers.rest_client module:

from dlt.sources.helpers import requests
from dlt.sources.helpers.rest_client import paginators

for page in requests.paginate(
    "https://api.example.com/endpoint",
    paginator=paginators.JSONResponsePaginator(next_url_path="metadata.nextPage"),
    auth=auth.APIKeyAuth(name="apikey", location="query", api_key="..."),
    params={"size": 100},
):
    print(page)

In my opinion, it would be logically simpler if the related parts lived under one package, e.g.:

from dlt.sources.helpers.rest_client import paginate, paginators

for page in paginate(
    "https://api.example.com/endpoint",
    paginator=paginators.JSONResponsePaginator(next_url_path="metadata.nextPage"),
    auth=auth.APIKeyAuth(name="apikey", location="query", api_key="..."),
    params={"size": 100},
):
    print(page)

@burnash burnash self-assigned this Apr 11, 2024
@burnash burnash added the enhancement New feature or request label Apr 11, 2024
@rudolfix
Copy link
Collaborator

@burnash yeah it probably does not make sense anymore. my plan was to make something self-contained where you can use literals to indicate which auth/paginator you want and then they autoconfigure or you configure them in toml. that was meant for quick ad hoc pipelines - similar how you'd use requests for prototyping.
but if we think that in 90% of cases we need to instantiate paginator/auth clases then it does not make sense

btw. maybe we could support same thing as requests accept as auth? actually it should be trivial

@rudolfix rudolfix added the sprint Marks group of tasks with core team focus at this moment label Apr 25, 2024
@rudolfix
Copy link
Collaborator

@burnash following on the above: let's close it. probably my fault to push the idea but it looked good at that point :)

@burnash burnash closed this Apr 25, 2024
@rudolfix rudolfix deleted the enh/add_paginate_to_helpers_requests branch May 20, 2024 19:16
@rudolfix rudolfix removed the sprint Marks group of tasks with core team focus at this moment label Aug 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants