Skip to content

Fix Next page pagination x extraction path#16

Merged
alexbourret merged 7 commits intomasterfrom
bug/sc-76640-next-page-pagination-with-extractionn-path
Dec 14, 2021
Merged

Fix Next page pagination x extraction path#16
alexbourret merged 7 commits intomasterfrom
bug/sc-76640-next-page-pagination-with-extractionn-path

Conversation

@alexbourret
Copy link
Copy Markdown
Collaborator

No description provided.

@shortcut-integration
Copy link
Copy Markdown

Copy link
Copy Markdown
Contributor

@JaneBellaiche JaneBellaiche left a comment

Choose a reason for hiding this comment

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

Bug fixed !
Another comment tho : a ValueError is raised if the extraction path is not found, but not if the Key to Next request URL is not found, is that expected ? Someone could have made a typo and if the first page is very big, he won't be able to see his mistake before really look into the output dataset

Comment thread custom-recipes/api-connect/recipe.json
Comment thread python-lib/pagination.py Outdated
Comment thread python-lib/pagination.py
@alexbourret
Copy link
Copy Markdown
Collaborator Author

@JaneBellaiche regarding the lack of error raised when Key to Next request URL is not found: it's a tricky one, because we don't know for sure how the targeted API reacts when there is no more page to be loaded. Some might keep the key and leave it empty, others will drop the key. And I can't raise on first page only: the targeted API might report a variable amount of data, which means extra page on day 1 but not an day 2...

Copy link
Copy Markdown
Contributor

@JaneBellaiche JaneBellaiche left a comment

Choose a reason for hiding this comment

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

LGTM !

@alexbourret alexbourret merged commit 805e14d into master Dec 14, 2021
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.

2 participants