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.

Sign up for GitHub

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

Jump to bottom

RESTClient: Extend docstrings of paginator classes #1207

Merged
merged 5 commits into from
Apr 25, 2024
Merged

RESTClient: Extend docstrings of paginator classes #1207

merged 5 commits into from
Apr 25, 2024

Conversation

burnash
Copy link
Collaborator

@burnash burnash commented Apr 10, 2024

Description

This PR extends class and method docstrings in the paginator classes of the RESTClient module and adds usage examples.

Related Issues

@burnash burnash self-assigned this Apr 10, 2024
@burnash burnash added documentation Improvements or additions to documentation enhancement New feature or request labels Apr 10, 2024
@netlify Netlify
Copy link

netlify bot commented Apr 10, 2024 •
edited
Loading

✅ Deploy Preview for dlt-hub-docs ready!

Name Link
🔨 Latest commit 3fe37ae
🔍 Latest deploy log https://app.netlify.com/sites/dlt-hub-docs/deploys/662a3f5e6a4924000890dc31
😎 Deploy Preview https://deploy-preview-1207--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.

AstrakhantsevaAA
AstrakhantsevaAA reviewed Apr 10, 2024
View reviewed changes
Copy link
Contributor

@AstrakhantsevaAA AstrakhantsevaAA left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Amazing! The most useful part is a response example like:

with a 'next' key like this:
{"items": [...], "pagination": {"next": "https://api.example.com/items?page=2"}}.

I would add them for every type of paginators (e.g. HeaderLinkPaginator)

rudolfix
rudolfix requested changes Apr 23, 2024
View reviewed changes
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.

I have one comment regarding next_reference. I'm not sure if it belongs to base class but if it is we should tell the user when to set it

dlt/sources/helpers/rest_client/paginators.py

Args:
response (Response): The response object from the API.
response (Response): The response object from the API request.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This method must call next_reference setter with a value of next reference right? otherwise has_next_page is not going to work correctly. Maybe we should mention this here?

I also wonder if we really need to force next_referece in base class and just leave has_next_page abstract? This property is kind of useful in BaseNextUrlPaginator (and I wonder why it is public?)

Copy link
Collaborator

Choose a reason for hiding this comment

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

otherwise this is really high quality!

Copy link
Collaborator Author

@burnash burnash Apr 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This method must call next_reference setter with a value of next reference right? otherwise has_next_page is not going to work correctly. Maybe we should mention this here?

I also wonder if we really need to force next_referece in base class and just leave has_next_page abstract? This property is kind of useful in BaseNextUrlPaginator (and I wonder why it is public?)

Yes, both are valid points:

  1. next_reference is useless in offset/limit pagination (or page-based), so having it in the base class is not a good design.
    Let's address it in a follow-up PR.
  2. update_state needs to modify _has_next_page indeed. I'll add this to the docstring.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@rudolfix please see #1280 that addresses (1)

@burnash burnash mentioned this pull request Apr 25, 2024
Merged
@rudolfix rudolfix added the sprint Marks group of tasks with core team focus at this moment label Apr 25, 2024
rudolfix
rudolfix previously approved these changes Apr 25, 2024
View reviewed changes
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.

LGTM!

however you need to solve conflict then use your admin power to merge or ask me for another review @burnash

@burnash burnash force-pushed the enh/docs/rest_client_paginator_docstrings branch from 332ac05 to 3fe37ae Compare April 25, 2024 11:32
@burnash burnash merged commit a93e577 into devel Apr 25, 2024
46 of 48 checks passed
@rudolfix rudolfix deleted the enh/docs/rest_client_paginator_docstrings branch May 20, 2024 19:15
@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
Reviewers

@AstrakhantsevaAA AstrakhantsevaAA AstrakhantsevaAA left review comments

@rudolfix rudolfix rudolfix left review comments

Assignees

@burnash burnash

Labels
documentation Improvements or additions to documentation enhancement New feature or request
Projects
dlt core library
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@burnash @rudolfix @AstrakhantsevaAA